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

[Federation] Make --dns-provider mandatory for kubefed #42092

Merged

Conversation

marun
Copy link
Contributor

@marun marun commented Feb 25, 2017

Targets #40757

I thought about adding a test for this but I decided it wasn't worth it. There's too much setup involved in being able to run Complete for such a simple change.

Release note:

The --dns-provider argument of 'kubefed init' is now mandatory and does not default to `google-clouddns`. To initialize a Federation control plane with Google Cloud DNS, use the following invocation: 'kubefed init --dns-provider=google-clouddns'

cc: @kubernetes/sig-federation-pr-reviews @madhusudancs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 25, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Feb 25, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@marun marun force-pushed the kubefed-init-dns-mandatory branch from 1edfe44 to 77fcf5f Compare February 25, 2017 05:08
@madhusudancs madhusudancs self-assigned this Feb 26, 2017
@madhusudancs
Copy link
Contributor

@marun this deserves a release note. Please add it.


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@madhusudancs
Copy link
Contributor

/lgtm
/approve


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 26, 2017
@marun marun force-pushed the kubefed-init-dns-mandatory branch from 77fcf5f to d1b6192 Compare February 27, 2017 16:49
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 27, 2017
@marun
Copy link
Contributor Author

marun commented Feb 27, 2017

@madhusudancs Is the PR title acceptable as a release note? Or can you point me to docs for what is expected?

@marun marun changed the title kubefed: make --dns-provider mandatory [Federation] Make --dns-provider mandatory for kubefed Feb 27, 2017
@madhusudancs
Copy link
Contributor

@marun I usually expect people to write a separate release note unless it is really unnecessary.

PR title, like commit logs/messages, need to be imperative, i.e. an instructional message describing the "why of your change". This post - https://chris.beams.io/posts/git-commit/ is a nice write about writing good commit logs, which also applies to PR titles.

On the other hand, a release note should be informative. It is an information to the user who does not really care about the details or "why of the change".

There are cases where these two are the same, but not in this PR. Your PR title is good as a, well, PR title. But for a user reading our release notes, it is not that useful. So I think this still deserves a release note. I would write the release note for this PR as:

`kubefed init`'s --dns-provider is now a mandatory flag and does not default to `google-clouddns` anymore. If you were previously using Google Cloud DNS and depending on the defaulting behavior, you should now explicitly pass `--dns-provider=google-clouddns` in the future invocations of `kubefed init`.

Btw, this is a release-note-action-required.

@madhusudancs
Copy link
Contributor

@k8s-bot gci gce e2e test this
@k8s-bot non-cri e2e test this

@madhusudancs
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: k8s-merge-robot, madhusudancs, marun

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 27, 2017
@madhusudancs madhusudancs added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 28, 2017
@enisoc enisoc added this to the v1.6 milestone Feb 28, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 41937, 41151, 42092, 40269, 42135)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. 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.

7 participants