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.

Sign up for GitHub

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

Jump to bottom

adding annotation-service and annotation-revision to kn service create/update #1029

Merged
merged 1 commit into from
Sep 30, 2020
Merged

adding annotation-service and annotation-revision to kn service create/update #1029

merged 1 commit into from
Sep 30, 2020

Conversation

arghya88
Copy link
Contributor

Description

Add "--annotation-service" and "--annotation-revision" flag to to kn service create and update providing ability to set specific annotations on service and revision

Changes

  • Add "--annotation-service" and "--annotation-revision" to kn service create/update

Reference

Fixes #1014

/lint

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 25, 2020
knative-prow-robot
knative-prow-robot reviewed Sep 25, 2020
View reviewed changes
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@arghya88: 0 warnings.

In response to this:

Description

Add "--annotation-service" and "--annotation-revision" flag to to kn service create and update providing ability to set specific annotations on service and revision

Changes

  • Add "--annotation-service" and "--annotation-revision" to kn service create/update

Reference

Fixes #1014

/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.

@knative-prow-robot
Copy link
Contributor

Welcome @arghya88! It looks like this is your first PR to knative/client 🎉

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 25, 2020
@knative-prow-robot
Copy link
Contributor

Hi @arghya88. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 25, 2020
@arghya88
Copy link
Contributor Author

/assign @sixolet

@dsimansk
Copy link
Contributor

Thanks looks good @arghya88 !

Could you please take a look on unit and e2e tests to add coverage for those specific flags?

/ok-to-test
/cc @navidshaikh

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2020
@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 14fd953 to 6ae5fec Compare September 25, 2020 13:12
@arghya88 arghya88 changed the title adding annotation-service and annotation-revision to kn create/update [WIP]adding annotation-service and annotation-revision to kn create/update Sep 25, 2020
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2020
@arghya88 arghya88 changed the title [WIP]adding annotation-service and annotation-revision to kn create/update adding annotation-service and annotation-revision to kn create/update Sep 25, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2020
@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 6ae5fec to cc75a22 Compare September 25, 2020 14:12
@arghya88
Copy link
Contributor Author

/retest

@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from cc75a22 to a0ead62 Compare September 25, 2020 15:51
@arghya88
Copy link
Contributor Author

/retest

1 similar comment
@arghya88
Copy link
Contributor Author

/retest

@arghya88 arghya88 changed the title adding annotation-service and annotation-revision to kn create/update adding annotation-service and annotation-revision to kn service create/update Sep 26, 2020
@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 549e9d5 to d3cff08 Compare September 28, 2020 09:54
@arghya88
Copy link
Contributor Author

/retest

1 similar comment
@arghya88
Copy link
Contributor Author

/retest

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020
@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 11131ff to 47f78ed Compare September 29, 2020 15:32
@arghya88
Copy link
Contributor Author

/retest

@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 47f78ed to 451faf1 Compare September 30, 2020 03:58
@arghya88
Copy link
Contributor Author

/retest

2 similar comments
@arghya88
Copy link
Contributor Author

/retest

@arghya88
Copy link
Contributor Author

/retest

@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 451faf1 to 8a62501 Compare September 30, 2020 04:37
@arghya88
Copy link
Contributor Author

/retest

2 similar comments
@arghya88
Copy link
Contributor Author

/retest

@arghya88
Copy link
Contributor Author

/retest

@navidshaikh
Copy link
Collaborator

this test needs fix log:ref

    service_options_test.go:167: create and validate service and revision annotations
    service_options_test.go:278: assertion failed: 
        --- gotRevisionAnnotations
        +++ expectedRevisionAnnotations
          map[string]string{
        - 	"client.knative.dev/user-image": "gcr.io/knative-boskos-70/client-e2e-img/16051/helloworld:latest",
          	"rev":                           "helloworld-rev",
          }

@arghya88 arghya88 force-pushed the annotation-serviceandrevision branch from 8a62501 to ab6c073 Compare September 30, 2020 08:54
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/service/configuration_edit_flags.go 83.2% 83.5% 0.3

@navidshaikh
Copy link
Collaborator

/retest

    result_collector.go:75: ERROR: Error: ReconcileIngressFailed: Ingress reconciliation failed

@arghya88
Copy link
Contributor Author

/retest

1 similar comment
@arghya88
Copy link
Contributor Author

/retest

@navidshaikh
Copy link
Collaborator

/retest

phew...

ERROR: timeout waiting for pods to come up

@arghya88
Copy link
Contributor Author

/retest

1 similar comment
@arghya88
Copy link
Contributor Author

/retest

navidshaikh
navidshaikh approved these changes Sep 30, 2020
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/lgtm

added a suggestion to update error message, which can be done in a subsequent PR

pkg/kn/commands/service/configuration_edit_flags.go

for key, value := range annotationServiceFlagMap {
if strings.HasPrefix(key, autoscaling.GroupName) {
return fmt.Errorf("Service cannot have annotation: %s", key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("Service cannot have annotation: %s", key)
return fmt.Errorf("service can not have auto-scaling related annotation: %s , please update using '--annotation-revision'", key)

Copy link
Collaborator

Choose a reason for hiding this comment

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

can be done in a subsequent PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure...I will create a new follow up PR to address this.

pkg/kn/commands/service/create_mock_test.go
"--annotation-service", autoscaling.InitialScaleAnnotationKey+"=1",
"--no-wait", "--revision-name=")
assert.Assert(t, err != nil)
assert.Assert(t, util.ContainsAll(output, "Service cannot have annotation: autoscaling.knative.dev/initialScale"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

the error message would need update following aforementioned suggestion

(also ContainsAll can work with array of words, so we should check presence of the words in the error message than the complete sentence)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will take care of this in the new follow up PR.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arghya88, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@arghya88
Copy link
Contributor Author

/test pull-knative-client-integration-tests-latest-release

@navidshaikh
Copy link
Collaborator

/retest

  result_collector.go:75: ERROR: Error: ReconcileIngressFailed: Ingress reconciliation failed

@knative-prow-robot knative-prow-robot merged commit e8ae0e1 into knative:master Sep 30, 2020
rhuss pushed a commit to rhuss/knative-client that referenced this pull request Oct 5, 2020
@arghya88 @rhuss
adding annotation-service and annotation-revision to kn create/update (…
b25c302 
…knative#1029)
@arghya88 arghya88 deleted the annotation-serviceandrevision branch November 28, 2020 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@dsimansk dsimansk dsimansk left review comments

@rhuss rhuss rhuss left review comments

@maximilien maximilien maximilien requested changes

@navidshaikh navidshaikh navidshaikh approved these changes

Assignees

@maximilien maximilien

@rhuss rhuss

@sixolet sixolet

@navidshaikh navidshaikh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add "--annotation-service" and "--annotation-revision" to kn service create/update
9 participants
@arghya88 @knative-prow-robot @dsimansk @maximilien @navidshaikh @knative-metrics-robot @rhuss @sixolet @googlebot