-
Notifications
You must be signed in to change notification settings - Fork 48
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 Cluster UID in Loadbalancer name and Droplet target tag #233
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: prksu 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me in general.
@prksu should we try to accompany the change with an e2e test to guarantee it's working for sure?
I realize this would effectively implement #185. If that sounds like too much work, we could also punt on that for now (but should probably have one before the next release IMO), and maybe do a quick manual test instead. WDYT?
@timoreimann so far I just do a manual test and it works as expected. $ kubectl get cluster -A
NAMESPACE NAME PHASE
default capdo-quickstart Provisioned
dev capdo-quickstart Provisioned
$ doctl compute load-balancer list --format ID,Name,Status,Tag
ID Name Status Tag
aca64495-63b9-48c6-9ea9-59d9df65c45d capdo-quickstart-apiserver-f7777de0-315c-466d-a9de-07121680b75f active sigs-k8s-io:capdo:capdo-quickstart:f7777de0-315c-466d-a9de-07121680b75f:apiserver
5309ee41-34ea-4e3d-9802-a955af256156 capdo-quickstart-apiserver-b72a8475-c71e-4fe4-b73a-b98c81afcf0c active sigs-k8s-io:capdo:capdo-quickstart:b72a8475-c71e-4fe4-b73a-b98c81afcf0c:apiserver And yes, it would be good to add e2e for this, let's make e2e as a follow-up. the test would be creating two capdo clusters with the same cluster-name in the different namespace. |
@prksu makes total sense to me. Thanks a bunch for driving this one. 👍 /lgtm |
What this PR does / why we need it:
This PR adds Cluster UID in Loadbalancer name and adds an additional tag that contains Cluster UID for droplet target tag to allow creating two or more clusters with the same name but different namespace in the single management cluster.
Which issue(s) this PR fixes:
Fixes #217
Special notes for your reviewer:
/cc @timoreimann
Release note: