Adding --tls option to domain create command #1419

vyasgun · 2021-08-05T10:55:43Z

Description

Support for TLS optionn in domain create using --tls flag

Changes

Added the new flag
Added unit tests for the new flag

Reference

Fixes #1408

knative-prow-robot

@vyasgun: 0 warnings.

In response to this:

Description

Support for TLS optionn in domain create

Changes

Added the new flag

Added unit tests for the new flag

Reference

Fixes #1408

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.

codecov · 2021-08-05T10:59:20Z

Codecov Report

Merging #1419 (053d265) into main (4691210) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
+ Coverage   78.68%   78.70%   +0.02%     
==========================================
  Files         162      162              
  Lines        8359     8368       +9     
==========================================
+ Hits         6577     6586       +9     
  Misses       1098     1098              
  Partials      684      684

Impacted Files	Coverage Δ
pkg/kn/commands/domain/domain.go	`95.45% <ø> (ø)`
pkg/kn/commands/domain/create.go	`79.48% <100.00%> (+3.01%)`	⬆️
pkg/serving/v1alpha1/client.go	`82.69% <100.00%> (+1.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4691210...053d265. Read the comment docs.

CHANGELOG.adoc

pkg/kn/commands/domain/create.go

Co-authored-by: David Simansky <dsimansk@redhat.com>

dsimansk · 2021-08-05T13:45:05Z

Thanks @vyasgun!
/lgtm

maximilien

Thanks for the contribution
/ok-to-test

maximilien · 2021-08-09T21:16:50Z

/lgtm

rhuss

Thanks, looks good to me ! I have some minor proposal wrt/ to naming, maybe we could change this so that we can add this PR to the upcoming release 0.25 (due today :) ?

docs/cmd/kn_domain_create.md

pkg/kn/commands/domain/create.go

rhuss · 2021-08-10T06:25:29Z

pkg/kn/commands/domain/domain.go

@@ -44,6 +44,7 @@ func NewDomainCommand(p *commands.KnParams) *cobra.Command {

 type RefFlags struct {
 	reference string
+	tls       string


happy to keep it, but maybe certSecret would be more appropriate ?

I think tls might be better because it corresponds to the flag name (--tls).

pkg/serving/v1alpha1/client.go

Co-authored-by: Roland Huß <rhuss@redhat.com>

rhuss

Thanks !

/approve
/lgtm

rhuss · 2021-08-10T07:58:02Z

/retest

Looks like there is some flake in the e2e tests, which might be due to recreating the same domain too fast after it has been deleted. @vyasgun good we adjust the test to use a different domain for the --tls test, so that we can avoid the race condition in the future ?

      ┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
        🦆 kn domain delete hello.example.com --namespace kne2etests12
        ┃ Domain mapping 'hello.example.com' deleted in namespace 'kne2etests12'.
        ┃ 
        ┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
        🦆 kn domain create hello.example.com --ref hello --tls tls-secret --namespace kne2etests12
        ┃ 
        🔥 Error: object is being deleted: domainmappings.serving.knative.dev "hello.example.com" already exists
        🔥 Run 'kn --help' for usage
        🔥 
        =================================================================

dsimansk · 2021-08-10T09:07:10Z

Thanks!

/approve
/lgtm

knative-prow-robot · 2021-08-10T09:07:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss, vyasgun

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

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

rhuss · 2021-08-10T09:32:30Z

The new error might be also a race condition, e.g. describe too quick after the domain has been created. I wonder whether we should also add a sync-wait when creating the domain in a future enhancement. For now, I would try to add some hardcoded delay between create and describe.

Also, I wonder whether the serving controller checks, if the referenced secret really exists (not sure if you have created in the test, too ?)

vyasgun · 2021-08-10T09:32:42Z

/retest

dsimansk · 2021-08-10T09:35:03Z

And maybe it's working well only for example.com? I wonder if newdomain.com shouldn't be rather e.g. new.example.com.

Because of ClusterDomainClaim???

vyasgun · 2021-08-10T09:36:35Z

@rhuss no it doesn't check if the secret exists. As per the referenced PR for the serving side change, the secret should be created if it doesn't exist. I haven't tested what happens in the backend but mentioning a TLS secret name in the spec changes the URL to https

knative-metrics-robot · 2021-08-10T11:00:32Z

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/domain/create.go	85.2%	87.1%	1.9
pkg/serving/v1alpha1/client.go	88.4%	89.4%	1.0

dsimansk · 2021-08-10T11:39:22Z

/retest

dsimansk · 2021-08-10T11:41:37Z

/lgtm

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 5, 2021

knative-prow-robot reviewed Aug 5, 2021

View reviewed changes

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 5, 2021

vyasgun force-pushed the pr/tls branch from f1be5e7 to abdb321 Compare August 5, 2021 10:57

Adding --tls option to domain create command

3d8d3fd

vyasgun force-pushed the pr/tls branch from abdb321 to 3d8d3fd Compare August 5, 2021 11:40

dsimansk reviewed Aug 5, 2021

View reviewed changes

CHANGELOG.adoc Outdated Show resolved Hide resolved

dsimansk reviewed Aug 5, 2021

View reviewed changes

pkg/kn/commands/domain/create.go Outdated Show resolved Hide resolved

Adding client test for builder

4dc9329

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 Aug 5, 2021

dsimansk reviewed Aug 5, 2021

View reviewed changes

pkg/kn/commands/domain/create.go Show resolved Hide resolved

vyasgun and others added 2 commits August 5, 2021 17:37

Update CHANGELOG.adoc

aa1dbe1

Co-authored-by: David Simansky <dsimansk@redhat.com>

Edited domain create usage message

1e09831

knative-prow-robot assigned dsimansk Aug 5, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

maximilien reviewed Aug 9, 2021

View reviewed changes

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

knative-prow-robot assigned maximilien Aug 9, 2021

rhuss reviewed Aug 10, 2021

View reviewed changes

Update docs/cmd/kn_domain_create.md

663aad1

Co-authored-by: Roland Huß <rhuss@redhat.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021

vyasgun and others added 2 commits August 10, 2021 12:29

Update pkg/serving/v1alpha1/client.go

472f461

Co-authored-by: Roland Huß <rhuss@redhat.com>

Update docs and usage message

dfb0610

vyasgun requested a review from rhuss August 10, 2021 07:19

rhuss approved these changes Aug 10, 2021

View reviewed changes

knative-prow-robot assigned rhuss Aug 10, 2021

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 10, 2021

Changing test domain name in e2e for tls test

f29d141

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021

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

dsimansk mentioned this pull request Aug 10, 2021

Deprecate lookup-path option and updated relevant documentation #1422

Merged

fix e2e domain create test case

7f80ced

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 10, 2021

Merge branch 'main' into pr/tls

053d265

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

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

knative-prow-robot merged commit ce790b4 into knative:main Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding --tls option to domain create command #1419

Adding --tls option to domain create command #1419

vyasgun commented Aug 5, 2021 •

edited

Loading

knative-prow-robot left a comment

codecov bot commented Aug 5, 2021 •

edited

Loading

dsimansk commented Aug 5, 2021

maximilien left a comment

maximilien commented Aug 9, 2021

rhuss left a comment

rhuss Aug 10, 2021

vyasgun Aug 10, 2021

rhuss left a comment

rhuss commented Aug 10, 2021

dsimansk commented Aug 10, 2021

knative-prow-robot commented Aug 10, 2021

rhuss commented Aug 10, 2021

vyasgun commented Aug 10, 2021

dsimansk commented Aug 10, 2021 •

edited

Loading

vyasgun commented Aug 10, 2021

knative-metrics-robot commented Aug 10, 2021

dsimansk commented Aug 10, 2021

dsimansk commented Aug 10, 2021

Adding --tls option to domain create command #1419

Adding --tls option to domain create command #1419

Conversation

vyasgun commented Aug 5, 2021 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Aug 5, 2021 • edited Loading

Codecov Report

dsimansk commented Aug 5, 2021

maximilien left a comment

Choose a reason for hiding this comment

maximilien commented Aug 9, 2021

rhuss left a comment

Choose a reason for hiding this comment

rhuss Aug 10, 2021

Choose a reason for hiding this comment

vyasgun Aug 10, 2021

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

rhuss commented Aug 10, 2021

dsimansk commented Aug 10, 2021

knative-prow-robot commented Aug 10, 2021

rhuss commented Aug 10, 2021

vyasgun commented Aug 10, 2021

dsimansk commented Aug 10, 2021 • edited Loading

vyasgun commented Aug 10, 2021

knative-metrics-robot commented Aug 10, 2021

dsimansk commented Aug 10, 2021

dsimansk commented Aug 10, 2021

vyasgun commented Aug 5, 2021 •

edited

Loading

codecov bot commented Aug 5, 2021 •

edited

Loading

dsimansk commented Aug 10, 2021 •

edited

Loading