fixes(#606): adds --cluster-local / --no-cluster-local flags #629

maximilien · 2020-02-01T00:35:28Z

When specified on 'service create' the --cluster-local flag will
make the created service 'private' by setting its config visibility
to cluster-local. This is done with label:

serving.knative.dev/visibility: cluster-local

The --no-cluster-local will remove the label.

Fixes #606

Proposed Changes

add --cluster-local and --no-cluster-local
when --cluster-local is specified then create service and add label: serving.knative.dev/visibility: cluster-local
add unit test for creating service with --cluster-local

When specified on 'service create' the '--cluster-local' flag will make the created service 'private' by setting its config visibility to 'cluster-local'. This is done with label: serving.knative.dev/visibility: cluster-local The --no-cluster-local will remove the label.

maximilien · 2020-02-01T00:37:19Z

/ok-to-test

rhuss

Thanks ! I one minor suggestion, see below.

Also, I think it would be cool if we could add already an E2E test to this functionality. would you mind to add a simpe one (or extend an existing one ?)

CHANGELOG.adoc

rhuss · 2020-02-01T08:48:08Z

pkg/kn/commands/service/create_test.go

+	}
+
+	_, present := template.Labels[config.VisibilityLabelKey]
+	assert.Assert(t, !present)


Could we please also check for the proper value of the annotation ? Maybe also in update_test.go that the annotation gets removed when using --no-cluster-local to complete the scenario.

Actually, slight problem adding the update_test.go. Seems like removing that label causes an API error:

Internal error occurred: admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: Saw the following changes without a name change (-old +new): spec.configuration.template {*v1alpha1.RevisionTemplateSpec}.ObjectMeta.Labels: -: "map[serving.knative.dev/visibility:cluster-local]" +: "map[]"

Let me know if you see something obvious. Unless my cluster is not up to date. Digging more.

The issue is, that the test uses the BYO feature for revision ("Bring your own" revision name). This means you need to to change the revision name to something unique on your own (like increasing a counter). Alternatively, you can use the auto revision name generation feature from Knative serving (which results in this random prefixes, like for pods of a deployment). You can servers side name generation with -revision-name "" (which might arguable not be the best option to use). Not sure anymore why we have settled on client side provided names by default.

However, I would expect that kn update deals with percularity. Could you show me how you did the test ? Also, have you set the proper flag that the flag that you set also creates a new revision ? (look in the flags file how other do it)

docs/cmd/kn_service_create.md

duglin · 2020-02-01T12:08:15Z

Just a minor comment on the help text, but otherwise LGTM

rhuss · 2020-02-01T13:57:42Z

@maximilien there is a compile error in the tests:

# knative.dev/client/pkg/kn/commands/service [knative.dev/client/pkg/kn/commands/service.test]
pkg/kn/commands/service/create_test.go:514:32: undefined: config

maximilien · 2020-02-03T19:02:53Z

/hold

maximilien · 2020-02-03T19:04:44Z

@maximilien there is a compile error in the tests:

# knative.dev/client/pkg/kn/commands/service [knative.dev/client/pkg/kn/commands/service.test]
pkg/kn/commands/service/create_test.go:514:32: undefined: config

Yup. Surprised did not see this locally. Must be lost in the test output. Thanks for pasting the location.

navidshaikh · 2020-02-04T03:24:11Z

/retest

Failed to successfully execute 'kn service create echo0 --image gcr.io/knative-samples/helloworld-go -n kne2etests6': Execution error: stderr: 'timeout: service 'echo0' not ready after 600 seconds

navidshaikh

Created a private service and subsequent update to service with only --cluster-local flag generates
new revisions (while the service was already private). It should see that service is already private and consider configuration change as nil?

➜  client git:(issue606) ./kn service create private --image gcr.io/knative-samples/helloworld-go --cluster-local
Creating service 'private' in namespace 'default':

  0.298s Configuration "private" is waiting for a Revision to become ready.
  9.714s ...
  9.769s Ingress has not yet been reconciled.
 50.790s Ready to serve.

Service 'private' created to latest revision 'private-lpcgx-1' is available at URL:
http://private.default.svc.cluster.local

➜  client git:(issue606) ./kn revision list
NAME              SERVICE   TRAFFIC   TAGS   GENERATION   AGE   CONDITIONS   READY   REASON
private-lpcgx-1   private   100%             1            69s   4 OK / 4     True    

➜  client git:(issue606) ./kn service update private --cluster-local
Updating Service 'private' in namespace 'default':

  9.866s Traffic is not yet migrated to the latest revision.
  9.894s Ingress has not yet been reconciled.
 11.257s Ready to serve.

Service 'private' updated to latest revision 'private-vygmw-2' is available at URL:
http://private.default.svc.cluster.local

➜  client git:(issue606) ./kn revision list                         
NAME              SERVICE   TRAFFIC   TAGS   GENERATION   AGE   CONDITIONS   READY   REASON
private-vygmw-2   private   100%             2            16s   4 OK / 4     True    
private-lpcgx-1   private                    1            97s   3 OK / 4     True    

➜  client git:(issue606) ./kn service update private --cluster-local
Updating Service 'private' in namespace 'default':

  9.883s Traffic is not yet migrated to the latest revision.
  9.919s Ingress has not yet been reconciled.
 11.278s Ready to serve.

Service 'private' updated to latest revision 'private-dgtsx-3' is available at URL:
http://private.default.svc.cluster.local

➜  client git:(issue606) ./kn revision list                         
NAME              SERVICE   TRAFFIC   TAGS   GENERATION   AGE    CONDITIONS   READY   REASON
private-dgtsx-3   private   100%             3            15s    4 OK / 4     True    
private-vygmw-2   private                    2            36s    4 OK / 4     True    
private-lpcgx-1   private                    1            117s   3 OK / 4     True

CHANGELOG.adoc

pkg/kn/commands/service/configuration_edit_flags.go

maximilien · 2020-02-04T23:05:22Z

/unhold

maximilien · 2020-02-04T23:08:10Z

/hold cancel

maximilien · 2020-02-04T23:27:59Z

/test pull-knative-client-integration-tests

maximilien · 2020-02-04T23:37:35Z

/test pull-knative-client-integration-tests

maximilien · 2020-02-04T23:41:44Z

Looks like we are getting errors in e2e tests and it seems to consistently failing. Should not be my doing (I hope) since the commend does not use --cluster-local or --no-cluster-local:

Running 'kn service create testsvc0 --image gcr.io/knative-samples/helloworld-go -n kne2etests4'...
--- PASS: TestSourceListTypes (8.02s)
    --- PASS: TestSourceListTypes/List_available_source_types (0.22s)
    --- PASS: TestSourceListTypes/List_available_source_types_in_YAML_format (0.14s)
Failed to successfully execute 'kn service create testsvc0 --image gcr.io/knative-samples/helloworld-go -n kne2etests8': Execution error: stderr: 'timeout: service 'testsvc0' not ready after 600 seconds
' error: 'exit status 1'FAIL	knative.dev/client/test/e2e	602.359s
FAIL
Finished run, return code is 1

cc: @navidshaikh @rhuss

test/e2e/service_test.go

navidshaikh

requested for few changes; those are causing e2e failures

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

rhuss · 2020-03-10T16:49:06Z

@maximilien I resolved the conflict, but as @navidshaikh points out the E2E issues are rooted within this PR. I suggest that we either fix or comment out the affected E2E test for now so that we can get in this feature for 0.13.0 right now and fix the E2E afterwards.

knative-metrics-robot · 2020-03-10T18:52:28Z

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	84.1%	84.6%	0.5

rhuss · 2020-03-10T19:01:42Z

gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

@mattmoor we are using gofmt with the same options with

find $(echo $source_dirs) -name "*.go" -print0 | xargs -0 gofmt -s -w

So, identical to your approach but more robust when it comes to file names which contains spaces.

Formatting is part of our regular build and also checked in CI that it has been called. It is performed via these functions (which also add goimports to the mix):

client/hack/build.sh

Lines 108 to 129 in 8dbf439

    
           go_fmt() { 
        
             echo "🧹 ${S}Format" 
        
             find $(echo $source_dirs) -name "*.go" -print0 | xargs -0 gofmt -s -w 
        
           } 
        
           source_format() { 
        
             set +e 
        
             which goimports >/dev/null 2>&1 
        
             if [ $? -ne 0 ]; then 
        
                echo "✋ No 'goimports' found. Please use" 
        
                echo "✋   go install golang.org/x/tools/cmd/goimports" 
        
                echo "✋ to enable import cleanup. Import cleanup skipped." 
        
                # Run go fmt instead 
        
                go_fmt 
        
             else 
        
                echo "🧽 ${X}Format" 
        
                goimports -w $(echo $source_dirs) 
        
                find $(echo $source_dirs) -name "*.go" -print0 | xargs -0 gofmt -s -w 
        
             fi 
        
             set -e 
        
           }

I suggest to switch off the format-check for the client repo as it leads to bogus suggestions as in #629 (comment)

rhuss · 2020-03-10T19:11:41Z

@maximilien there's a unit test error which you also should get with a hack/build.sh -t 👍

--- FAIL: TestServiceCreateWithClusterLocal (0.02s)
    create_test.go:560: assertion failed: expression is false: present

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/kn/commands/service/create_test.go

rhuss · 2020-03-10T19:32:55Z

I think the integration test fails for the same reason as the unit test, i.e. that the label is not properly set on the service.

knative-metrics-robot · 2020-03-10T19:33:27Z

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	84.1%	84.7%	0.7

knative-metrics-robot · 2020-03-10T19:38:28Z

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	84.1%	84.7%	0.7

maximilien · 2020-03-10T19:46:00Z

I think the integration test fails for the same reason as the unit test, i.e. that the label is not properly set on the service.

Yup. I see there is a case when labels are nil on service. Should be good on last update.

maximilien · 2020-03-10T19:47:28Z

@maximilien there's a unit test error which you also should get with a hack/build.sh -t 👍
--- FAIL: TestServiceCreateWithClusterLocal (0.02s)
    create_test.go:560: assertion failed: expression is false: present

Yeah, optimistically pushed and went to lunch. It's all passing locally now (at least).

maximilien · 2020-03-10T20:02:32Z

Yeah, finally all green @rhuss :)

rhuss · 2020-03-10T20:59:09Z

Thanks !

/approve
/lgtm

knative-prow-robot · 2020-03-10T20:59:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, 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:

~~OWNERS~~ [maximilien,rhuss]

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

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

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 1, 2020

knative-prow-robot requested review from evankanderson and sixolet February 1, 2020 00:35

maximilien force-pushed the issue606 branch from 64988b2 to e5e4bfc Compare February 1, 2020 00:37

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 1, 2020

rhuss requested changes Feb 1, 2020

View reviewed changes

duglin reviewed Feb 1, 2020

View reviewed changes

docs/cmd/kn_service_create.md Outdated Show resolved Hide resolved

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2020

Merge branch 'master' into issue606

df50f17

maximilien force-pushed the issue606 branch from 5c15f5d to df50f17 Compare February 3, 2020 23:50

navidshaikh requested changes Feb 4, 2020

View reviewed changes

CHANGELOG.adoc Outdated Show resolved Hide resolved

CHANGELOG.adoc Outdated Show resolved Hide resolved

pkg/kn/commands/service/configuration_edit_flags.go Outdated Show resolved Hide resolved

maximilien mentioned this pull request Feb 4, 2020

generated negative flag --no-xxx description are not well capitalized #636

Closed

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

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2020

updating from latest master and addressing review comments

83390dd

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2020

navidshaikh reviewed Mar 10, 2020

View reviewed changes

test/e2e/service_test.go Outdated Show resolved Hide resolved

navidshaikh reviewed Mar 10, 2020

View reviewed changes

test/e2e/service_test.go Show resolved Hide resolved

navidshaikh requested changes Mar 10, 2020

View reviewed changes

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2020

mattmoor reviewed Mar 10, 2020

View reviewed changes

maximilien force-pushed the issue606 branch from cdbf690 to 8c13b92 Compare March 10, 2020 18:49

maximilien force-pushed the issue606 branch from 8c13b92 to 2913867 Compare March 10, 2020 19:30

mattmoor reviewed Mar 10, 2020

View reviewed changes

pkg/kn/commands/service/create_test.go Outdated Show resolved Hide resolved

Merge branch 'master' into issue606

6fde086

maximilien force-pushed the issue606 branch from 2913867 to 6fde086 Compare March 10, 2020 19:35

knative-prow-robot assigned rhuss Mar 10, 2020

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

knative-prow-robot merged commit 2b9377d into knative:master Mar 10, 2020

rhuss mentioned this pull request Mar 13, 2020

Run plugin on Windows #738

Merged

maximilien deleted the issue606 branch March 27, 2020 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes(#606): adds --cluster-local / --no-cluster-local flags #629

fixes(#606): adds --cluster-local / --no-cluster-local flags #629

maximilien commented Feb 1, 2020 •

edited

Loading

maximilien commented Feb 1, 2020

rhuss left a comment

rhuss Feb 1, 2020

maximilien Feb 3, 2020

rhuss Feb 5, 2020

duglin commented Feb 1, 2020

rhuss commented Feb 1, 2020

maximilien commented Feb 3, 2020

maximilien commented Feb 3, 2020

navidshaikh commented Feb 4, 2020

navidshaikh left a comment

maximilien commented Feb 4, 2020 •

edited

Loading

maximilien commented Feb 4, 2020

maximilien commented Feb 4, 2020

maximilien commented Feb 4, 2020

maximilien commented Feb 4, 2020 •

edited

Loading

navidshaikh left a comment

mattmoor left a comment

mattmoor left a comment

rhuss commented Mar 10, 2020

knative-metrics-robot commented Mar 10, 2020

rhuss commented Mar 10, 2020

rhuss commented Mar 10, 2020

mattmoor left a comment

rhuss commented Mar 10, 2020

knative-metrics-robot commented Mar 10, 2020

knative-metrics-robot commented Mar 10, 2020

maximilien commented Mar 10, 2020

maximilien commented Mar 10, 2020

maximilien commented Mar 10, 2020

rhuss commented Mar 10, 2020

knative-prow-robot commented Mar 10, 2020

fixes(#606): adds --cluster-local / --no-cluster-local flags #629

fixes(#606): adds --cluster-local / --no-cluster-local flags #629

Conversation

maximilien commented Feb 1, 2020 • edited Loading

Proposed Changes

maximilien commented Feb 1, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss Feb 1, 2020

Choose a reason for hiding this comment

maximilien Feb 3, 2020

Choose a reason for hiding this comment

rhuss Feb 5, 2020

Choose a reason for hiding this comment

duglin commented Feb 1, 2020

rhuss commented Feb 1, 2020

maximilien commented Feb 3, 2020

maximilien commented Feb 3, 2020

navidshaikh commented Feb 4, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

maximilien commented Feb 4, 2020 • edited Loading

maximilien commented Feb 4, 2020

maximilien commented Feb 4, 2020

maximilien commented Feb 4, 2020

maximilien commented Feb 4, 2020 • edited Loading

navidshaikh left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

rhuss commented Mar 10, 2020

knative-metrics-robot commented Mar 10, 2020

rhuss commented Mar 10, 2020

rhuss commented Mar 10, 2020

mattmoor left a comment

Choose a reason for hiding this comment

rhuss commented Mar 10, 2020

knative-metrics-robot commented Mar 10, 2020

knative-metrics-robot commented Mar 10, 2020

maximilien commented Mar 10, 2020

maximilien commented Mar 10, 2020

maximilien commented Mar 10, 2020

rhuss commented Mar 10, 2020

knative-prow-robot commented Mar 10, 2020

maximilien commented Feb 1, 2020 •

edited

Loading

maximilien commented Feb 4, 2020 •

edited

Loading

maximilien commented Feb 4, 2020 •

edited

Loading