-
Notifications
You must be signed in to change notification settings - Fork 50
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
Upgrade operator-sdk to v1.39.1 #537
base: devel
Are you sure you want to change the base?
Conversation
Hi @balintTobik. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
/retest |
As this PR is removing kube-rbac-proxy, it is also addressing #483 |
name: default | ||
namespace: system | ||
- kind: ServiceAccount | ||
name: controller-manager |
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.
Who creates the controller-manager
service account?
The manager.yaml doesn't specify ServiceAccount
explicitly.
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.
Hi @bpradipt! Thanks for the comment!
If I understand correctly this service account was introduced in v1.5.0 sdk version (https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.5.0/#gov3-add-a-systemcontroller-manager-serviceaccount-to-your-project)
Do you know the "default" was kept by purpose? Or I can change it to new "controller-manager" SA in other rolebinding as well?
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.
Hi @balintTobik ! Thanks for working on this.
A new operator SDK release can introduce incompatibilities that call for very specific changes. Every operator SDK release thus comes with a migration guide at https://sdk.operatorframework.io/docs/upgrading-sdk-version/ .
This PR is doing a 3 release jump. Please break this commit down into 3 per-release commits. Each commit should at least provide the link to the corresponding migration guide as in a9d0f2b and any extra details that can help. This will greatly improve the reviewing experience and the overall maintainability.
It would also be easier to review if it were clearer which changes are manual as opposed to generated by the upgrade. |
aee5695
to
116742d
Compare
@@ -59,7 +59,8 @@ spec: | |||
- command: | |||
- /manager | |||
args: | |||
- --enable-leader-election | |||
- --metrics-bind-address=127.0.0.1:8080 | |||
- --leader-elect |
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 like this is fixing a bug from cbebd56 that turned enable-leader-election
into leader-elect
. This was a long long time ago so I am a bit surprised this wasn't discovered before. How did you spot it ? Code reading ? Hit some error ? Also, is metrics-bind-address
needed here ?
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.
When I tested my change, I faced an issue, try to use a non-existing argument. I saw that the it's changed in clusterserviceversion.yaml during bundle creation. It's interesting, because when I build it without these changes (with 1.36.0 operator-sdk), it didn't changed this arguments.
Maybe, because of removing/adding manifests to clusterserviceversion reload manager manifest as well...
Default, metrics-bind-address is ":8080", so I think it's needed.
Also this value is used currently: https://github.com/openshift/sandboxed-containers-operator/blob/devel/bundle/manifests/sandboxed-containers-operator.clusterserviceversion.yaml#L373
Following steps from https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.38.0/ Skip steps for upgrade 1.37.0, because it doesn't relevant. Signed-off-by: Balint Tobik <[email protected]>
Update manager container's arguments to make it compatible with the code Signed-off-by: Balint Tobik <[email protected]>
Following steps from https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.39.0/ No migration required upgrading 1.39.0 -> 1.39.1 Signed-off-by: Balint Tobik <[email protected]>
116742d
to
1c184a2
Compare
@balintTobik: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
- Description of the problem which is fixed/What is the use case
Upgrade operator-sdk to v1.39.1 version.
- What I did
Update operator-sdk version in docs/DEVELOPMENT.md
Update versions in Makefile.
Remove codes related to deprecated kube-rbac-proxy.
Add new network-policy related changes.
- How to verify it
- Description for the changelog
Upgrade operator-sdk to v1.39.1.
Closes: KATA-3521