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

Add a feature flag for formatting assets #2968

Merged

Conversation

justinsb
Copy link
Member

Image rewriting involves a yaml format of the manifests, which makes for
a large and hard to read diff. Add a feature flag to disable it, along
with a workaround to the release notes.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 17, 2017
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Code looks good, I understand the tweak. Release notes which are super important are a bit confusing ;(


* Manifests are rewritten by default, which includes a normalization phase.
This can make it hard to understand the actual changes (as opposed to just the formatting changes).
A feature flag has been added, `export KOPS_FEATURE_FLAGS="-RewriteManifests"` which can be used
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the purpose of this it actually is. To the uninitiated it looks like you are running an update on a cluster twice. Can we add more context or pull?

Copy link
Member Author

Choose a reason for hiding this comment

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

You should try kops update on an existing cluster, with master

Copy link
Contributor

Choose a reason for hiding this comment

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

I have and that is what you need to mention ;) Because of change x we did, when running on an existing cluster, you will get y behavior.

@justinsb justinsb force-pushed the make_manifest_rewrite_optional branch 2 times, most recently from 7298ebc to 1cd8b49 Compare July 17, 2017 16:35
@justinsb
Copy link
Member Author

Now with enhanced release notes

@chrislovecnm
Copy link
Contributor

Your merge with builder.go did not go ok - pkg/assets/builder.go:22: imported and not used: "k8s.io/kops/vendor/github.com/golang/glog"

@justinsb
Copy link
Member Author

Can I have "LGTM subject to passing tests" please? Don't want to lose another day....

Image rewriting involves a yaml format of the manifests, which makes for
a large and hard to read diff.  Add a feature flag to disable it, along
with a workaround to the release notes.
@justinsb justinsb force-pushed the make_manifest_rewrite_optional branch from 1cd8b49 to 8b717cf Compare July 18, 2017 01:45
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Thanks

@chrislovecnm
Copy link
Contributor

/assign
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2017
@chrislovecnm
Copy link
Contributor

chrislovecnm commented Jul 18, 2017

Self merge if you are impatient ;)

@chrislovecnm chrislovecnm merged commit 43eede3 into kubernetes:master Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants