-
Notifications
You must be signed in to change notification settings - Fork 72
feat: Add function set-standard-labels
#930
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
base: master
Are you sure you want to change the base?
Conversation
7efdc4b
to
8aa6986
Compare
set-standard-labels
8aa6986
to
793eefd
Compare
793eefd
to
f218c94
Compare
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.
Some quick comments.
AppComponent = AppLabelPrefix + "component" | ||
AppPartOf = AppLabelPrefix + "part-of" | ||
AppManagedBy = AppLabelPrefix + "managed-by" | ||
) |
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.
May be we can call the function set-app-labels
since it's too specific to setting application labels.
type SetStandardLabels struct { | ||
forDeployment string | ||
// TBD: validate whether the recommended labels from the ResourceList.Items are set as expected. | ||
validateOnly bool |
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.
Would be nice to have a separate function (or just have the same function aliased) with validate-app-labels
will be nice.
} | ||
pkgName := kptfile.GetName() | ||
|
||
// Assign blueprint pkg name to "app.kubernetes.io/name" and deployment pkg name to "app.kubernetes.io/instance" |
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.
When I was exploring argocd, this label was being used by argocd
also for apps. Would be good to check if how well will it will play with argocd etc given that most of the users we are seeing are using kpt with argocd.
if len(forDeployments) == 0 { | ||
return false | ||
} | ||
return true |
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.
will be nice to have helper function in the SDK for this.
// that follows some kpt specific conventions: | ||
// - "app.kubernetes.io/name" comes from the current blueprint package name, or (if current is deployment) the upstream blueprint package. | ||
// - "app.kubernetes.io/instance" is only set for deployment package, using its package name. | ||
func (r *SetStandardLabels) Run(ctx *fn.Context, _ *fn.KubeObject, items fn.KubeObjects, results *fn.Results) bool { |
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 you plan to support a bit more flexible input format where label value can be sourced from any field ? or that fits better in regular set-labels
?
Pending on dependent PRs
set-labels
to be importable #929TODO:
write README, setup new fn release workflow