-
Notifications
You must be signed in to change notification settings - Fork 835
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 labels to deployments and improve label coverage of resources #2130
Conversation
Signed-off-by: glindsell <gl@seldon.io>
/test integration |
Tue Jul 14 16:15:05 UTC 2020 impatient try |
Tue Jul 14 16:15:13 UTC 2020 impatient try |
Tue Jul 14 16:15:21 UTC 2020 impatient try |
/test integration |
Wed Jul 15 08:22:44 UTC 2020 impatient try |
/test integration |
Thu Jul 16 08:36:47 UTC 2020 impatient try |
Thu Jul 16 08:36:59 UTC 2020 impatient try |
Thu Jul 16 08:37:39 UTC 2020 impatient try |
/test integration |
/test lint |
Thu Jul 16 15:21:12 UTC 2020 impatient try |
Thu Jul 16 15:21:19 UTC 2020 impatient try |
Signed-off-by: glindsell <gl@seldon.io>
/test integration |
Fri Jul 17 14:24:28 UTC 2020 impatient try |
Fri Jul 17 14:24:35 UTC 2020 impatient try |
Fri Jul 17 14:25:29 UTC 2020 impatient try |
/test integration |
Fri Jul 17 16:15:18 UTC 2020 impatient try |
/test integration |
Mon Jul 20 09:42:57 UTC 2020 impatient try |
Wed Jul 22 17:11:46 UTC 2020 impatient try |
/test integration |
Thu Jul 23 08:37:46 UTC 2020 impatient try |
Signed-off-by: glindsell <gl@seldon.io>
/test integration |
Thu Jul 23 13:26:31 UTC 2020 impatient try |
Thu Jul 23 13:26:50 UTC 2020 impatient try |
Thu Jul 23 13:27:22 UTC 2020 impatient try |
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.
Nice one @glindsell! I just added a small question down below, but everything else looks good! 👍
@@ -732,8 +735,9 @@ func createContainerService(deploy *appsv1.Deployment, | |||
SessionAffinity: corev1.ServiceAffinityNone, | |||
}, | |||
} | |||
addLabelsToService(svc, pu, p) | |||
addLabelsToDeployment(deploy, containerServiceKey, containerServiceValue) | |||
deploy.ObjectMeta.Labels[containerServiceKey] = containerServiceValue |
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.
Do we need to add a label to any service here? Just wondering as before we were also calling a addLabelsToService
method.
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.
addLabelsToService
doesn't add the containerServiceKey
or containerServiceValue
labels. Therefore when moving the addLabelsTo...
functions up a level to the createComponents
method I didn't add svc.ObjectMeta.Labels[containerServiceKey] = containerServiceValue
Perhaps this does need to be added? I'm not sure as it is not part of the set of labels that I am dealing with in this PR.
Label_shadow = "shadow" | ||
Label_canary = "canary" | ||
Label_explainer = "explainer" | ||
Label_router = "seldon.io/router" |
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.
Nice one! Namespacing the labels under seldon.io/*
seems like the right thing to do 👍
BTW @glindsell could you also add some comment to the |
Signed-off-by: glindsell <gl@seldon.io>
Fri Jul 24 16:09:43 UTC 2020 impatient try |
Fri Jul 24 16:10:00 UTC 2020 impatient try |
/test integration |
Fri Jul 24 16:17:08 UTC 2020 impatient try |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriangonz 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 |
/hold (briefly for the question) |
/hold |
/test integration |
Mon Jul 27 08:40:53 UTC 2020 impatient try |
@axsaucedo are we ok to merge now? |
/hold cancel |
Wed Jul 29 10:36:50 UTC 2020 impatient try |
Wed Jul 29 10:36:58 UTC 2020 impatient try |
Signed-off-by: glindsell gl@seldon.io
What this PR does / why we need it:
See this comment #1405 (comment)
Which issue(s) this PR fixes:
Fixes #1405
Fixes #2187
Special notes for your reviewer:
Does this PR introduce a user-facing change?: