-
Notifications
You must be signed in to change notification settings - Fork 263
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
feat(annotations): Adds annotation flag for service create and update #422
Conversation
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.
@navidshaikh: 0 warnings.
In response to this:
Fixes #327
Proposed Changes
- Adds specified annotations to service object meta and revision template meta
- Adds
--annotation
/-a
flag to service create and update options- User can specify '-' at the end of the annotation key to remove an annotation
- Adds unit and e2e tests
- Updates docs accordingly
/lint
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
} | ||
|
||
// UpdatMaxScale updates max scale annotation | ||
func UpdateMaxScale(template *servingv1alpha1.RevisionTemplateSpec, max int) error { | ||
return UpdateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max)) | ||
return UpdateRevisionTemplateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max)) |
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.
updated the name to be explicit
// UpdateAnnotation updates (or adds) an annotation to the given service | ||
func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) error { | ||
// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template. | ||
// Also validates the autoscaling annotation values |
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.
added note about validation
autoscaling.TargetAnnotationKey) | ||
} | ||
|
||
return UpdateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target)) |
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.
removed the pending TODO
d8d9068
to
9879293
Compare
This is ready for review. |
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.
Few small items...
a90621a
to
7c7fe64
Compare
/test pull-knative-client-go-coverage |
Added mock tests for service update with annotations, the negative go coverage delta should change now. |
docs/cmd/kn_service_create.md
Outdated
``` | ||
|
||
### Options | ||
|
||
``` | ||
-a, --annotation stringArray Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). |
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.
I'm not sure I would pick having the short -a
opt. I feel like those shoudl be used sparingly.
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.
I wasn't sure on using -a
too and was hoping for some feedback :+1
Its better to be explicit for annotations. Removing -a
.
Thanks!
- Adds specified annotations to service object meta and revision template meta - Adds --annotation / -a flag to service create and update options - User can specify '-' at the end of the annotation key to remove an annotation - Adds unit and e2e tests - Updates docs and changelog accordingly
9c296bc
to
c127519
Compare
The following is the coverage report on the affected files.
|
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.
Thanks !
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: navidshaikh, rhuss 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 |
…knative#422) * feat(annotations): Adds annotation flag for service create and update - Adds specified annotations to service object meta and revision template meta - Adds --annotation / -a flag to service create and update options - User can specify '-' at the end of the annotation key to remove an annotation - Adds unit and e2e tests - Updates docs and changelog accordingly * Adds example for service create with annotation * Adds mock unit tests for service update with annotations * Removes the short hand -a for annotation flag
Fixes #327
Proposed Changes
--annotation
/-a
flag to service create and update options/lint