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

revert defaulter-gen #1288

Merged
merged 10 commits into from
Apr 15, 2019

Conversation

theishshah
Copy link
Member

Description of the change:
Revert defaulter-gen changes made in #1050

Motivation for the change:
Discussion on #1038

@theishshah theishshah requested a review from hasbro17 April 4, 2019 20:05
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 4, 2019
@hasbro17
Copy link
Contributor

hasbro17 commented Apr 4, 2019

Can you reformat the commit message according to our guidelines.

Also add a line in the removed section of the CHANGELOG to point this out. And probably mention that defaulting for CRs should be done with mutating webhooks.

removes defaulter gen, instead users should use mutating webhooks

reverts pr operator-framework#1050
Track changes between releases
@joelanford
Copy link
Member

Should we also remove defaulter-gen references from internal/pkg/scaffold/gopkgtoml.go and sdk-cli-reference.md?

@hasbro17
Copy link
Contributor

hasbro17 commented Apr 5, 2019

@joelanford Yes good catch.
@theishshah Can you update the PR to remove the constraint for defaulter-gen from the scaffold:
https://github.com/operator-framework/operator-sdk/blob/master/internal/pkg/scaffold/gopkgtoml.go#L46
And update the example in the CLI reference as well:
https://github.com/operator-framework/operator-sdk/blob/master/doc/sdk-cli-reference.md#example-1

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2019
removes defaulter gen, instead users should use mutating webhooks

reverts pr operator-framework#1050
Track changes between releases
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 8, 2019
@hasbro17
Copy link
Contributor

hasbro17 commented Apr 8, 2019

You need to update gopkgtoml_test.go to remove "k8s.io/code-generator/cmd/defaulter-gen" from the required imports as well.

@@ -15,5 +15,5 @@
package version

var (
Version = "v0.6.0+git"
Version = "v0.7.0+git"
Copy link
Member

Choose a reason for hiding this comment

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

@theishshah I'm confused why some of these 0.7.0 changes are showing up in this PR. I checked master to be sure and all of them are already there. Maybe some weirdness happened with merging or rebasing after 0.7.0 was released?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you might want to squash merge onto master.

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 15, 2019
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. Just one nit.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: theishshah <ish@ishshah.me>
@theishshah theishshah merged commit a139fdf into operator-framework:master Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants