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

#5700: Add command line flag for disabling Subnet ELB tags #5875

Merged

Conversation

seanson
Copy link
Contributor

@seanson seanson commented Oct 4, 2018

This is a proposed fix/improvement for issue #5700.

The current actual kops behavior is to tag shared subnets, but the documentation refers to having to manage your own tags. I feel it is best to keep the current subnet tagging behavior and add a toggle to disable. This will keep user experience standard by creating working clusters when using existing subnets but also allows power users to manually manage tags if they wish. Manual management of tags allows things like using separate dedicated subnets for internal ingress.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 4, 2018
@seanson seanson force-pushed the 5700_add_flag_for_no_subnet_tags branch from e0a906e to ca073d0 Compare October 4, 2018 11:17
@seanson seanson changed the title 5700: Add command line flag for disabling Subnet ELB tags #5700: Add command line flag for disabling Subnet ELB tags Oct 4, 2018
@seanson
Copy link
Contributor Author

seanson commented Oct 8, 2018

/assign @andrewsykim

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2018
@seanson seanson force-pushed the 5700_add_flag_for_no_subnet_tags branch from ca073d0 to 737a7a2 Compare October 10, 2018 01:48
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2018
@idealhack
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 29, 2018
@chrisz100
Copy link
Contributor

/lgtm
/assign @justinsb @mikesplain
Thanks @seanson !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2018
Copy link
Contributor

@mikesplain mikesplain left a comment

Choose a reason for hiding this comment

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

Thanks so much @seanson!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, mikesplain, seanson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2018
@k8s-ci-robot k8s-ci-robot merged commit 5dce6b1 into kubernetes:master Nov 9, 2018
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants