Skip to content

bump controller-tools to v0.2.0 #1642

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

Conversation

estroz
Copy link
Member

@estroz estroz commented Jul 3, 2019

Description of the change: bump controller-tools to v0.2.0 and update CRD generator API/unit tests.

Motivation for the change: controller-tools k8s deps versions should match those of controller-runtime when possible. #1143 requires some changes in this release too.

Note: #1607 should be closed once the refactor base branch is merged with master.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2019
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 3, 2019
@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch 2 times, most recently from ba3db71 to c49ecbb Compare July 4, 2019 15:21
@estroz estroz changed the title [WIP] bump controller-tools to v0.2.0 bump controller-tools to v0.2.0 Jul 7, 2019
@estroz estroz requested review from AlexNPavel, lilic and hasbro17 July 7, 2019 01:16
@estroz estroz requested a review from lilic July 8, 2019 18:30
@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch from c86b140 to 8f7bfbd Compare July 8, 2019 19:12
@estroz
Copy link
Member Author

estroz commented Jul 8, 2019

Two changes that need discussion:

  1. CRD manifests can contain multiple specified versions, so CRD file naming should change from <group>_<version>_<kind>_crd.yaml to something else. For example, controller-tools uses <group>_<resource>.yaml for CRD's. To stick with our prior suffix convention and make it clear what the file contains I suggest <group>_<resource>_crd.yaml.
  2. The CRD generator permits legacy support for spec.version but not simultaneously with spec.versions. The checkCRDVersions() function permits both forms as long as spec.version is the first element in spec.versions, which is correct behavior. How should we handle this discrepancy? Do we need to do anything aside from log a warning about spec.version deprecation and error if the expected versioning spec is not met?

/cc @joelanford @AlexNPavel @hasbro17 @lilic

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2019
@hasbro17
Copy link
Contributor

hasbro17 commented Jul 9, 2019

@estroz Can we rebase or cherry pick #1652 into the refactor/controller-runtime-v0.2.0 branch?
And update the tag from // +genclient:nonNamespaced to // +kubebuilder:resource:path=<lowercase-kind-plural>,scope=Namespaced

Or do you want to do that as a separate PR?

@lilic
Copy link
Member

lilic commented Jul 9, 2019

@estroz For .1 I think we should follow the convention that kubebuilder has, so it will make it easier to migrate, do they have anything established already?

For 2. I think the log about the deprecation and error out if spec is not met, sounds good to me.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Can we update the CHANGELOG.

@hasbro17 if you are the one doing the migration guide we should make a note of the versions/version in there as well.

Otherwise lgtm

@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch from 9576718 to 6d4d6c3 Compare July 9, 2019 18:34
@estroz
Copy link
Member Author

estroz commented Jul 9, 2019

CRD file naming changes were quite extensive and touched both Helm and Ansible docs/tests. PTAL.

/cc @lilic @hasbro17 @AlexNPavel @joelanford @jmrodri @fabianvf

@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch 3 times, most recently from 237857f to 90d0b3f Compare July 9, 2019 22:31
@estroz
Copy link
Member Author

estroz commented Jul 10, 2019

@lilic kubebuilder would generate a CRD file name cache.example.com_memcacheds.yaml. IMO cache_memcacheds_crd.yaml looks much nicer and contains all the necessary uniqueness/information that the kubebuilder name gives. WDYT?

@lilic
Copy link
Member

lilic commented Jul 10, 2019

@lilic kubebuilder would generate a CRD file name cache.example.com_memcacheds.yaml. IMO cache_memcacheds_crd.yaml looks much nicer and contains all the necessary uniqueness/information that the kubebuilder name gives. WDYT?

Since we are moving to kubebuilder at some point, I would suggest we go the kubebuilder way as we probably will have to change it then anyways.

@estroz estroz force-pushed the bump-controller-tools-v0.2.0 branch from 26bcf4b to 3b49157 Compare July 10, 2019 23:09
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 5, 2019
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 5, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 12, 2019
@estroz estroz requested review from joelanford and hasbro17 August 12, 2019 18:03
@estroz estroz requested a review from joelanford August 26, 2019 18:28
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

@estroz estroz merged commit 4d330f3 into operator-framework:refactor/controller-runtime-v0.2.0 Aug 29, 2019
@estroz estroz deleted the bump-controller-tools-v0.2.0 branch August 29, 2019 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants