Skip to content
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

Patch Series to handle upgrading to latest version of operator-sdk #134

Merged
merged 25 commits into from
Sep 28, 2021

Conversation

bpradipt
Copy link
Contributor

- Description of the problem which is fixed/What is the use case
This patch series makes necessary changes to the code base to align with latest operator-sdk (v1.11.0) , controller-runtime (0.9.2) and kubernetes version (1.21.2)

- What I did
Updated code, dependencies etc

- How to verify it
Building and installing the cluster by following the developer documentation

- Description for the changelog

Upgrade to handle latest operator-sdk (v1.11.0) , controller-runtime (0.9.2) and kubernetes version (1.21.2)

@bpradipt
Copy link
Contributor Author

/assign @jensfr @pmores

@bpradipt
Copy link
Contributor Author

Seems the CI is failing due to change of operator name (OO_PACKAGE)

@jensfr
Copy link
Contributor

jensfr commented Sep 20, 2021

/retest

@pmores
Copy link
Contributor

pmores commented Sep 20, 2021

I won't pretend I'm able to review properly all of this :-) so I'll have just two questions:

  • with all the changes in Makefile, is the idea that its interface should still work the same? I mean, make podman-build IMG=<...> and podman-push and also make bundle-build BUNDLE_IMG=<...>.
  • there are changes in the series that would break the validating webhook (well unless the new SDK works radically differently from the previous one). However there's also some back-and-forth in the series about changing these files or not, so while I think the final state is good I'd like to ask whether you verified that the webhook still works (by just attempting to create a duplicate KataConfig)?

Thanks!

@jensfr
Copy link
Contributor

jensfr commented Sep 20, 2021

/retest

@bpradipt bpradipt force-pushed the sdk-upgrade branch 2 times, most recently from 7b97c33 to 3f30c8a Compare September 22, 2021 11:45
@bpradipt
Copy link
Contributor Author

/retest

3 similar comments
@bpradipt
Copy link
Contributor Author

/retest

@bpradipt
Copy link
Contributor Author

/retest

@bpradipt
Copy link
Contributor Author

/retest

@bpradipt bpradipt force-pushed the sdk-upgrade branch 2 times, most recently from 6889623 to 72262c5 Compare September 23, 2021 14:24
@bpradipt
Copy link
Contributor Author

/retest

@bpradipt
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021
@bpradipt bpradipt force-pushed the sdk-upgrade branch 4 times, most recently from 1d8adb4 to 11c416b Compare September 24, 2021 17:43
@bpradipt
Copy link
Contributor Author

I won't pretend I'm able to review properly all of this :-) so I'll have just two questions:

  • with all the changes in Makefile, is the idea that its interface should still work the same? I mean, make podman-build IMG=<...> and podman-push and also make bundle-build BUNDLE_IMG=<...>.

Installing podman-docker will ensure no changes to default Makefile targets. Otherwise for every target an equivalent podman-* will be needed. The development doc mentions podman-docker as a pre-req

  • there are changes in the series that would break the validating webhook (well unless the new SDK works radically differently from the previous one). However there's also some back-and-forth in the series about changing these files or not, so while I think the final state is good I'd like to ask whether you verified that the webhook still works (by just attempting to create a duplicate KataConfig)?

I tested webhook on a real cluster and it prevents multiple KataConfig from being created. I would also suspect that any issues will get covered in unit tests itself as we do have webhook based tests.

Thanks!

@bpradipt
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021
@jensfr jensfr requested review from jensfr and removed request for fidencio and harche September 28, 2021 07:56
Copy link
Contributor

@jensfr jensfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you @bpradipt !

@bpradipt bpradipt merged commit a96a040 into openshift:master Sep 28, 2021
@bpradipt bpradipt deleted the sdk-upgrade branch September 28, 2021 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants