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.

Sign up for GitHub

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

Jump to bottom

revert defaulter-gen #1288

Merged
merged 10 commits into from
Apr 15, 2019
Merged

revert defaulter-gen #1288

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.

revert defaulter-gen
b5af8b2 
removes defaulter gen, instead users should use mutating webhooks

reverts pr operator-framework#1050
@theishshah theishshah force-pushed the revert-defaultergen branch from 6cf3003 to b5af8b2 Compare April 4, 2019 20:25
update changelog
060a8da 
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
Ish Shah added 3 commits April 5, 2019 12:17
revert defaulter-gen
cb33f25 
removes defaulter gen, instead users should use mutating webhooks

reverts pr operator-framework#1050
update changelog
168c3cd 
Track changes between releases
Remove defaulter-gen from docs and scaffold
504c664
@estroz estroz mentioned this pull request Apr 8, 2019
Merged
@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.

joelanford
joelanford reviewed Apr 9, 2019
View reviewed changes
version/version.go Outdated
@@ -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
joelanford
joelanford approved these changes Apr 15, 2019
View reviewed changes
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.

@joelanford @theishshah
Update CHANGELOG.md
dce0471 
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
Reviewers

@estroz estroz estroz left review comments

@joelanford joelanford joelanford approved these changes

@hasbro17 hasbro17 Awaiting requested review from hasbro17

@AlexNPavel AlexNPavel Awaiting requested review from AlexNPavel

Assignees
No one assigned
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theishshah @hasbro17 @joelanford @estroz @openshift-ci-robot