-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update controller-runtime to v0.9.2 #267
Conversation
9db62f7
to
a034a0f
Compare
b14bcf1
to
22b3da9
Compare
CI is green 💚 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, and I don't see anything too concerning in the v0.9.0 release notes.
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.9.0
.github/workflows/ci.yml
Outdated
GO_VERSION: '1.14' | ||
GOLANGCI_VERSION: 'v1.31' | ||
GO_VERSION: '1.16' | ||
GOLANGCI_VERSION: 'v1.40.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change necessary? I generally like the idea of bumping the version of golangci-lint we're running, but unfortunately we actually run it multiple times during a build. One is triggered by make lint
and uses the version in the build submodule, which is v1.31. The other uses this GitHub Action. We run both because we like that the GitHub Action leaves comments on PRs with linter violations (and because I haven't got around to disabling make lint
being called as part of the other CI jobs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only way to make the CI happy. 🤔 Do you prefer to downgrade the linter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we'd bump to Go 1.16 but keep using golangci-lint 1.31. Can you provide some context on how that combination breaks CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, reverted that line, see the breaking CI: https://github.com/crossplane/crossplane-runtime/pull/267/checks?check_run_id=2817289334#step:7:74
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything we can do about this? (bumping the version fixes the issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted my change to fixup the CI again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saschagrunert I believe I had similar issues with CI in my PR, and able to work around with: crossplane-contrib/provider-helm@90bb4b3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, fixed as suggested and the CI seems green now.
22b3da9
to
f09ff2d
Compare
b9898e4
to
7849b18
Compare
7971b1f
to
b16ce80
Compare
@saschagrunert thanks a lot for working on this. I need this in provider-helm to bump the helm version. |
b16ce80
to
873ecc9
Compare
This patch updates the controller-runtime dependency to the latest release. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
873ecc9
to
18438ce
Compare
Description of your changes
This patch updates the controller-runtime dependency to the latest release.
Closes #265
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested