Add option for adding labels only to service and/or template #703

shashwathi · 2020-03-05T20:58:02Z

Description

Added command line flags label-service and label-revision to add labels more granularly for service and revision template objects.

Fixes #675

/lint

cc @andrew-su
/assign @rhuss

In this PR we are introducing 2 additional flags for setting label for service(label-service) and revision(label-revision) only. Signed-off-by: Andrew Su <[email protected]>

Signed-off-by: Andrew Su <[email protected]>

knative-prow-robot

@shashwathi: 0 warnings.

In response to this:

Description

Added command line flags label-service and label-revision to add labels more granularly for service and revision template objects.

Fixes #675

/lint

cc @andrew-su
/assign @rhuss

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.

shashwathi · 2020-03-05T23:57:17Z

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

shashwathi · 2020-03-05T23:57:24Z

/test pull-knative-client-integration-tests

rhuss · 2020-03-06T13:40:47Z

@shashwathi thanks a lot for the PR ! The test is failing because the test cluster is already on the latest Knative eventing with some breaking changes. We need the latest update to be merged in to get this PR in.

Please stay tuned.

rhuss · 2020-03-08T10:24:46Z

/retest

rhuss

Thanks a lot, looks good in general !

I have some comments, how the implementation could be optimised for better readability and maintainability.

As I would like to get in this PR for 0.13.0 I suggest that we merge this right now and we can improve then on it even post 0.13.0. Only for the suggested API change I would submit and additional PR right now so that it get into 0.13.0 (as the API is exposed as part of the release, too).

In another PR I would also love to see some more tests:

Unit tests for testing the overriding rules
Unit tests also for the command line option
E2E tests for the various labelling cases.

@shashwathi Wdyt ?

rhuss · 2020-03-09T09:33:25Z

pkg/kn/commands/service/configuration_edit_flags.go

+		serviceMap.Merge(labelsAllMap)
+		revisionMap.Merge(labelsAllMap)
+
+		labelServiceMap, err := util.MapFromArrayAllowingSingles(p.LabelService, "=")


we could put the duplicate code into a single method called twice. This time we can guarantee that the specific labels are treated the same.

rhuss · 2020-03-09T09:33:48Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -47,6 +47,8 @@ type ConfigurationEditFlags struct {
 	AutoscaleWindow            string
 	Port                       int32
 	Labels                     []string
+	LabelService               []string


would calls it "LabelsService" (plural, like for Labels)

rhuss · 2020-03-09T09:33:56Z

pkg/kn/commands/service/configuration_edit_flags.go

@@ -47,6 +47,8 @@ type ConfigurationEditFlags struct {
 	AutoscaleWindow            string
 	Port                       int32
 	Labels                     []string
+	LabelService               []string
+	LabelRevision              []string


rhuss · 2020-03-09T09:35:12Z

pkg/serving/config_changes.go

-var ServiceOnlyLabels = map[string]struct{}{
-	serving.GroupName + "/visibility": {},
-}
+// // ServiceOnlyLabels should only appear on the Service and NOT on the


Please remove it not needed anymore.

rhuss · 2020-03-09T09:45:12Z

pkg/serving/config_changes.go


 // UpdateLabels updates the labels identically on a service and template.
 // Does not overwrite the entire Labels field, only makes the requested updates
-func UpdateLabels(service *servingv1.Service, template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error {
+func UpdateLabels(service *servingv1.Service, template *servingv1.RevisionTemplateSpec,


Having functions with more than 4 or 5 arguments is a code smell, especially when the have the same types. This is not good API design and confuses users and code readers. Instead, its better to either split up a 'god' function (i.e. in a simpler method which might then be called twice) or introduce an option struct (which allows for "named" parameters (field of the structure) in favor of positional arguments). If possible the formere solution is easier to implement and more lightweight. The latter (option struct) should be used only if it can't be avoid.

This seems kind of a nit-pick comment for now, but maintaining god methods that do too much is really hard. Also using smaller method can help in guaranteeing consistency.

E.g. one could also use two methods UpdateServiceLabels or UpdateRevisionTemplateLabels (or maybe also UpdatePodLabels as this is where the lable will end up and might be easier to understand).

I think this is the better option.

rhuss · 2020-03-09T12:46:49Z

pkg/kn/commands/service/configuration_edit_flags.go

+			"To unset, specify the label name followed by a \"-\" (e.g., name-). This flag takes "+
+			"precedence over \"label\" flag.")
+	p.markFlagMakesRevision("label-service")
+	command.Flags().StringArrayVarP(&p.LabelRevision, "label-revision", "", []string{},


Shouldn't it be really --label-pod instead of --label-revision as it is about adding the label to the pod spec ?

googlebot · 2020-03-09T16:11:12Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

andrew-su · 2020-03-09T18:50:03Z

@googlebot I consent.

googlebot · 2020-03-09T18:50:10Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Signed-off-by: Shash Reddy <[email protected]>

knative-metrics-robot · 2020-03-09T19:24:00Z

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.9%	84.1%	0.2
pkg/serving/config_changes.go	79.9%	79.6%	-0.3

shashwathi · 2020-03-09T19:27:37Z

@rhuss : Yes we will follow up with another Pr for unit and integration test cases. Thanks for the feedback on the PR. Its ready for another review 👍
cc @andrew-su

rhuss · 2020-03-09T21:01:26Z

thanks looks good ! Let's merge this know and follow up with more tests after the release.

I added issue #718 to not forget.

/lgtm

rhuss · 2020-03-09T21:01:39Z

/approve

knative-prow-robot · 2020-03-09T21:01:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, shashwathi

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~~ [rhuss]

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

* Don't tear down test environment on boskos * Add comments

shashwathi added 3 commits March 5, 2020 12:50

Add option for adding labels only to service and/or template

bfdf123

In this PR we are introducing 2 additional flags for setting label for service(label-service) and revision(label-revision) only. Signed-off-by: Andrew Su <[email protected]>

Update docs

4a51bf6

Signed-off-by: Andrew Su <[email protected]>

Add changelog entry

d6e44be

Signed-off-by: Andrew Su <[email protected]>

knative-prow-robot assigned rhuss Mar 5, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 5, 2020

knative-prow-robot reviewed Mar 5, 2020

View reviewed changes

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 5, 2020

knative-prow-robot requested review from cppforlife and maximilien March 5, 2020 20:58

Merge branch 'master' into issue-675

18b7353

rhuss reviewed Mar 9, 2020

View reviewed changes

Refactor UpdateLabels method to be more generic.

79e273c

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Mar 9, 2020

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Mar 9, 2020

andrew-su added 2 commits March 9, 2020 15:11

Refactored labels common code.

6c64dbc

Signed-off-by: Shash Reddy <[email protected]>

Pass pointer refernce instead of copy to update labels

a6f66ed

rhuss mentioned this pull request Mar 9, 2020

Add e2e tests and command line tests for "--labels" options #718

Closed

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2020

knative-prow-robot merged commit 81c1d9c into knative:master Mar 9, 2020

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Don't tear down test environment on boskos (knative#703)

691caf5

* Don't tear down test environment on boskos * Add comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option for adding labels only to service and/or template #703

Add option for adding labels only to service and/or template #703

shashwathi commented Mar 5, 2020

knative-prow-robot left a comment

shashwathi commented Mar 5, 2020

shashwathi commented Mar 5, 2020

rhuss commented Mar 6, 2020

rhuss commented Mar 8, 2020

rhuss left a comment •

edited

Loading

rhuss Mar 9, 2020

rhuss Mar 9, 2020

rhuss Mar 9, 2020

rhuss Mar 9, 2020

rhuss Mar 9, 2020

rhuss Mar 9, 2020

rhuss Mar 9, 2020

googlebot commented Mar 9, 2020

andrew-su commented Mar 9, 2020

googlebot commented Mar 9, 2020

knative-metrics-robot commented Mar 9, 2020

shashwathi commented Mar 9, 2020

rhuss commented Mar 9, 2020

rhuss commented Mar 9, 2020

knative-prow-robot commented Mar 9, 2020

Add option for adding labels only to service and/or template #703

Add option for adding labels only to service and/or template #703

Conversation

shashwathi commented Mar 5, 2020

Description

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

shashwathi commented Mar 5, 2020

shashwathi commented Mar 5, 2020

rhuss commented Mar 6, 2020

rhuss commented Mar 8, 2020

rhuss left a comment • edited Loading

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

rhuss Mar 9, 2020

Choose a reason for hiding this comment

googlebot commented Mar 9, 2020

andrew-su commented Mar 9, 2020

googlebot commented Mar 9, 2020

knative-metrics-robot commented Mar 9, 2020

shashwathi commented Mar 9, 2020

rhuss commented Mar 9, 2020

rhuss commented Mar 9, 2020

knative-prow-robot commented Mar 9, 2020

rhuss left a comment •

edited

Loading