Add --service-account-name flag #401

igsong · 2019-09-04T21:29:37Z

Proposed Changes

Add a flag --service-account-name to set serviceAccountName property of ksvc

Release Note

Add --service-account flag

knative-prow-robot · 2019-09-04T21:29:52Z

Hi @igsong. 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.

navidshaikh · 2019-09-05T09:29:55Z

/ok-to-test

navidshaikh · 2019-09-06T10:33:49Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -107,7 +108,8 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 		"keep the running image for the service constant when not explicitly specifying "+
 			"the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)")
 	// Don't mark as changing the revision.
-
+	command.Flags().StringVar(&p.ServiceAccountName, "service-account-name", "-", "Service account name. To unset, specify \"-\".")


any particular reason we are using default as - instead of nil?

I've changed the default value to an empty string.

navidshaikh · 2019-09-06T10:37:18Z

Addition of new flag should be highlighted in release notes, can you please append a line in https://github.com/knative/client/blob/master/CHANGELOG.adoc ?
rebase needed.

maximilien

We also need an e2e integration test for this feature.

maximilien · 2019-09-06T19:57:13Z

docs/cmd/kn_service_create.md

+      --requests-cpu string           The requested CPU (e.g., 250m).
+      --requests-memory string        The requested memory (e.g., 64Mi).
+      --revision-name string          The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --service-account-name string   Service account name. To unset, specify "-". (default "-")


Let’s make clearer with “To unset specify “-“ as the value”

Also would this be clearer with “” instead of “-“?

I agree. I will make the change.

How can you unset something when you create a service ? Isn't it just good enough to leave out the option if you don't want to create a service account being set ?

I've changed the description of the flag with the change of the name of the flag.

maximilien · 2019-09-06T19:58:47Z

pkg/serving/config_changes.go

+// UpdateServiceAccountName updates the service account name used for the corresponding knative service
+func UpdateServiceAccountName(template *servingv1alpha1.RevisionTemplateSpec, serviceAccountName string) error {
+	serviceAccountName = strings.TrimSpace(serviceAccountName)
+	if serviceAccountName == "-" {


Related to my comment above in description. Empty string for unsettling would make this read better

I've changed the default value to an empty string.

rhuss · 2019-09-10T10:43:23Z

docs/cmd/kn_service_update.md

+      --requests-cpu string           The requested CPU (e.g., 250m).
+      --requests-memory string        The requested memory (e.g., 64Mi).
+      --revision-name string          The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --service-account-name string   Service account name. To unset, specify "-". (default "-")


Does this mean, that when you use kn service update without this option, that the service account will be always removed because the default is - ? I would expect that there is no default and service account name is left untouched when this option is not given.

For kn service update I would prefer:

--service-account new-name : Update the service account with the new name

--service-account : Clear the service account

Option not given : Don't touch service account

@rhuss I've made a change according to your comments. Please check it out and, if there are additional comments, please let me know.

…ge its default value from "-" to an empty string. In addition, CHANGELOG.adoc is changed.

knative-metrics-robot · 2019-09-11T19:00:28Z

The following is the coverage report on pkg/.
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.7%	83.7%	-0.0
pkg/serving/config_changes.go	80.7%	81.2%	0.5

navidshaikh

Looks good to me. Keeping it open if there is any additional feedback.

navidshaikh

/lgtm

knative-prow-robot · 2019-09-23T09:48:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: igsong, navidshaikh

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:

~~OWNERS~~ [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

navidshaikh · 2019-09-23T09:48:50Z

We'll need service describe to list the service account info if present.

rhuss · 2019-09-23T10:00:22Z

docs/cmd/kn_service_create.md

@@ -58,6 +58,7 @@ kn service create NAME --image IMAGE [flags]
      --requests-cpu string      The requested CPU (e.g., 250m).
      --requests-memory string   The requested memory (e.g., 64Mi).
      --revision-name string     The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --service-account string   Service account name to set. Empty service account name will result to clear the service account.


I know, it's a bit nit-picky, but using a --service-account without arg during a kn service create is a no-op and it won't clear the service account (as there is one). I know that this option is shared, but maybe we can use still different help messages for create & update ?

happy to get this PR merged now, but maybe we can adjust this in a later PR.

navidshaikh · 2019-09-23T10:48:41Z

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

navidshaikh · 2019-09-23T11:17:54Z

/retest

This changeset introduces the coveragecalculator package to the webhook based API Coverage tool. This package contains wire-contract that the webhook server would expose to any client using the tool to get API coverage data. README.md file has more information. The change also introuces the concept of rules to provide a mechanism to control resource tree traversal.

Add --service-account-name flag

be40b03

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 4, 2019

knative-prow-robot requested review from cppforlife and mattmoor September 4, 2019 21:29

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 4, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2019

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 5, 2019

navidshaikh reviewed Sep 6, 2019

View reviewed changes

maximilien suggested changes Sep 6, 2019

View reviewed changes

rhuss reviewed Sep 10, 2019

View reviewed changes

igsong added 2 commits September 11, 2019 10:20

Merge branch 'master' of https://github.com/knative/client into sa_flag

05f0a48

Change the flag --service-account-name to --service-account, and chan…

68da1eb

…ge its default value from "-" to an empty string. In addition, CHANGELOG.adoc is changed.

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2019

Update docs with ./hack/build.sh

1553dfc

navidshaikh approved these changes Sep 13, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2019

navidshaikh approved these changes Sep 23, 2019

View reviewed changes

knative-prow-robot assigned navidshaikh Sep 23, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2019

rhuss reviewed Sep 23, 2019

View reviewed changes

knative-prow-robot merged commit 14ec594 into knative:master Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --service-account-name flag #401

Add --service-account-name flag #401

igsong commented Sep 4, 2019 •

edited by navidshaikh

Loading

knative-prow-robot commented Sep 4, 2019

navidshaikh commented Sep 5, 2019

navidshaikh Sep 6, 2019 •

edited

Loading

igsong Sep 11, 2019

navidshaikh Sep 13, 2019

navidshaikh commented Sep 6, 2019

maximilien left a comment

maximilien Sep 6, 2019

igsong Sep 7, 2019

rhuss Sep 10, 2019

igsong Sep 11, 2019

maximilien Sep 6, 2019

igsong Sep 11, 2019

rhuss Sep 10, 2019

rhuss Sep 10, 2019

igsong Sep 11, 2019

knative-metrics-robot commented Sep 11, 2019

navidshaikh left a comment

navidshaikh left a comment

knative-prow-robot commented Sep 23, 2019

navidshaikh commented Sep 23, 2019

rhuss Sep 23, 2019

navidshaikh commented Sep 23, 2019

navidshaikh commented Sep 23, 2019

Add --service-account-name flag #401

Add --service-account-name flag #401

Conversation

igsong commented Sep 4, 2019 • edited by navidshaikh Loading

Proposed Changes

knative-prow-robot commented Sep 4, 2019

navidshaikh commented Sep 5, 2019

navidshaikh Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navidshaikh commented Sep 6, 2019

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Sep 11, 2019

navidshaikh left a comment

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 23, 2019

navidshaikh commented Sep 23, 2019

Choose a reason for hiding this comment

navidshaikh commented Sep 23, 2019

navidshaikh commented Sep 23, 2019

igsong commented Sep 4, 2019 •

edited by navidshaikh

Loading

navidshaikh Sep 6, 2019 •

edited

Loading