add git ops options #1122

itsmurugappan · 2020-11-17T18:23:04Z

Description

Adds Gitops mode where in the changes are made in a local directory instead of a remote cluster

Changes

This PR only enables the option for kn service create/delete/update/list/describe
Gitops client implementing the knservingclient interface
Client injection is based on the command
* Tests pending
e2e pending

Need reveiwers input on the below

When '--in-dir' flag is provided this mode is enabled
Named the new client gitops client

Reference

Partially fixes #30

/hold

cc @rhuss @navidshaikh @dsimansk @maximilien

knative-prow-robot

@itsmurugappan: 4 warnings.

In response to this:

Description

Adds Gitops mode where in the changes are made in a local directory instead of a remote cluster

Changes

This PR only enables the option for kn service create/delete/update/list/describe

Gitops client implementing the knservingclient interface

Client injection is based on the command

Tests pending

Need reveiwers input on the below

When '--in-dir' flag is provided this mode is enabled

Named the new client gitops client

Reference

Partially fixes #30

/hold

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.

pkg/kn/commands/human_readable_flags.go

pkg/serving/v1/gitops.go

maximilien

Good start. Important feature. Thanks for starting 💯

maximilien · 2020-11-17T23:05:29Z

pkg/kn/commands/revision/revision_test.go

@@ -68,7 +68,7 @@ func executeRevisionCommand(client clientservingv1.KnServingClient, args ...stri

 	output := new(bytes.Buffer)
 	knParams.Output = output
-	knParams.NewServingClient = func(namespace string) (clientservingv1.KnServingClient, error) {
+	knParams.NewServingClient = func(namespace string, cmd *cobra.Command) (clientservingv1.KnServingClient, error) {


What about typing this CreateClient(...) function type?

pkg/kn/commands/types.go

pkg/serving/v1/gitops.go

knative-prow-robot · 2020-11-17T23:09:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: itsmurugappan, maximilien

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]

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

rhuss · 2020-12-01T18:19:23Z

@itsmurugappan sorry that I couldn't review it yet. This week is still crazy busy for me, but let me give it a quick review at least about the UX tomorrow.

itsmurugappan · 2020-12-07T16:05:39Z

tests are passing and unit tests are added

rhuss

Thanks a lot and sorry for the late reply.

I like the simplicity of the implementation but have some stylistic comment though.
There are probably some edge case, and we should "cleanup" the service from the empty / default value before writing it out (same as we do the for kn service apply command)

Not sure about the name of the option though, and maybe we should allow also to operate on a single file (eg. with an option that points to a file instead of a directory).

The directory option is nice because it will allow to add other objects (e.g. from eventing) into the mix.

Let's discuss tomorrow the naming a bit, I need to sleep over it ;-)

pkg/serving/v1/gitops.go

pkg/kn/commands/types.go

pkg/serving/v1/gitops.go

pkg/serving/v1/gitops_test.go

rhuss

Except for the tmpdir calculation, looks good to me ! We still need to think about a proper option name, as I think that in-dir is not descriptive enough.

What's about:

--offline-dir (emphasizing that we are 'offline')
--local-dir (emphasizing that no cluster is used, but lacks an indication that its for the output)
--target-dir (emphasizing that it is generating something in the directory)

Or we could also have something like --offline / --local together with the --target-dir to make it even clearer that we:

Don't connect to a cluster
Create something in a target-dir. We could default to "." for the target dir if not given.
Allows also to add later an --target-file which then would point to a single file to update.

Instead of target we could also use output (e.g. (--output-dir, --output (for a file) or --output-file. However, --output (or -o) is already kind of occupied by the output format for the read-only commands, so I probably wouldn't reuse it in this context. (but --target, --target-dir would be also possible)

Finally we might think if the extra namespace directory level is needed. E.g do we expect to create many services across namespaces or is the major use case to create a single service or multiple services in the same namespace. Because if the latter is the case, we might first implement a single file argument --target mysvcl.yml and later maybe add options for bulk operations.

wdyt ?

rhuss

Also, while trying it out, I think we need to tune the output message as this still is focussed on cluster-side creation. E.g. we see an empty URL (which is natural), and we might need to adapt / check certain output methods that they can work with an empty status: field.

itsmurugappan · 2020-12-08T18:00:51Z

Finally we might think if the extra namespace directory level is needed. E.g do we expect to create many services across namespaces or is the major use case to create a single service or multiple services in the same namespace. Because if the latter is the case, we might first implement a single file argument --target mysvcl.yml and later maybe add options for bulk operations.

IMO having namespaces will have the following benefits

Organized
Scenario where this command is used to generate files and checkin to GitHub , followed by ci process creating the services, namespaces might be handy
Supports listing
from kn argument perspective we still have the one argument

itsmurugappan · 2020-12-08T18:01:54Z

Except for the tmpdir calculation, looks good to me ! We still need to think about a proper option name, as I think that in-dir is not descriptive enough.

What's about:

--offline-dir (emphasizing that we are 'offline')

--local-dir (emphasizing that no cluster is used, but lacks an indication that its for the output)

--target-dir (emphasizing that it is generating something in the directory)

I like offline-dir

dsimansk · 2020-12-09T13:05:55Z

My preferential list would be:

--offline-dir
--target-dir
--local-dir

However, I wonder if we need to have *-dir suffix at all. E.g. to have a bit shorter flag name without dash, just --offline flag.
With the following resolution:

default: ./ - write to current dir
path/to/dir - write everything to the directory
path/to/file.yaml - write to single file

rhuss · 2020-12-09T19:23:03Z

Yeah, I discussed this with @itsmurugappan on slack and my current preference would be --target (as "offline" sounds so negative ;-P

And also I think we can use a single option for both modes that we might want to support:

I think we should not create the top-level dir automatically and when we use --target something then, if something points to a directory we do it like implemented, but if not, and something ends with .yaml , .yml, or .json we create the appropriate file (even if it does not exist).

So for the directory mode, we would require an existing directory (the structure within the directory can then be built up by us)

For the file mode, we require a proper extension and create the file if needed.

dsimansk · 2020-12-09T22:31:23Z

Yeah, I discussed this with @itsmurugappan on slack and my current preference would be --target (as "offline" sounds so negative ;-P

@rhuss @itsmurugappan +1 sounds well thought through.

lgtm-com · 2020-12-11T00:10:29Z

This pull request introduces 1 alert when merging 3fd6e3b into bfd1834 - view on LGTM.com

new alerts:

1 for Missing error check

itsmurugappan · 2020-12-11T16:41:40Z

@rhuss @dsimansk all comments have been addressed, please take a look.

knative-metrics-robot · 2020-12-11T22:14:24Z

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/create.go	84.7%	85.0%	0.3
pkg/kn/commands/service/delete.go	87.2%	87.5%	0.3
pkg/kn/commands/service/describe.go	83.8%	83.9%	0.1
pkg/kn/commands/service/list.go	93.8%	93.9%	0.2
pkg/kn/commands/service/service.go	91.7%	88.9%	-2.8
pkg/kn/commands/service/update.go	92.5%	92.7%	0.3
pkg/kn/commands/types.go	55.9%	51.6%	-4.4
pkg/serving/v1/gitops.go	Do not exist	83.3%

dsimansk · 2020-12-15T12:01:43Z

pkg/kn/commands/human_readable_flags.go

+
+// AddGitOpsFlags adds flags to enable gitops mode
+func AddGitOpsFlags(flags *pflag.FlagSet) {
+	flags.String("target", "", "work on local directory instead of a remote cluster")


A little formatting nit to align with the rest.

Suggested change

flags.String("target", "", "work on local directory instead of a remote cluster")

flags.String("target", "", "Work on local directory instead of a remote cluster.")

On a second thought I wonder if the message shouldn't be more descriptive/verbose. The following is just an example feel free to change or ignore it. :)

Create Knative resources in local directory or file instead of a remote cluster. Default mode is current directory, in addition path to custom directory or file extensions .yaml, .yml, .json are accepted.

@rhuss I recall you've mentioned the feature is experimental. Therefore we should add (experimental) suffix at the end of description, right?

Yes, we should mark it as experimental and we should definitely include it for 0.20

To not lose to much time, lets merge this and create a followup PR to add the indication that this feature is experimental in a followup PR.

rhuss · 2020-12-15T17:15:24Z

@itsmurugappan thanks a ton ! hope to jump on a next round of review tomorrow or the day after. End of the year is always crazy busy, but I will be there next week, too, when things are slowing down. So confident to get this landed within this year still !

rhuss · 2021-01-14T12:07:41Z

/lgtm

let's merge it now to get it into 0.20 and address the remaining minor issues (marking as "experimental" in the description in a new PR (which we should also merge right before 0.20)

rhuss · 2021-01-14T12:10:01Z

/unhold

add git ops options

02916ab

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 17, 2020

knative-prow-robot requested review from cppforlife and sixolet November 17, 2020 18:23

knative-prow-robot reviewed Nov 17, 2020

View reviewed changes

pkg/kn/commands/human_readable_flags.go Show resolved Hide resolved

pkg/serving/v1/gitops.go Outdated Show resolved Hide resolved

pkg/serving/v1/gitops.go Show resolved Hide resolved

pkg/serving/v1/gitops.go Outdated Show resolved Hide resolved

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

add git ops options

410e36f

maximilien approved these changes Nov 17, 2020

View reviewed changes

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

add unit tests

95b4e37

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2020

itsmurugappan added 5 commits December 6, 2020 10:14

add unit tests

e8c8977

add unit tests

47f9232

add unit test

7fbdeb0

add unit test

f6b8ac5

add unit test

9852e28

rhuss reviewed Dec 7, 2020

View reviewed changes

pkg/serving/v1/gitops.go Outdated Show resolved Hide resolved

pkg/kn/commands/types.go Outdated Show resolved Hide resolved

pkg/serving/v1/gitops.go Outdated Show resolved Hide resolved

pkg/serving/v1/gitops.go Outdated Show resolved Hide resolved

review comments

db8e20f

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 7, 2020

dsimansk reviewed Dec 8, 2020

View reviewed changes

pkg/serving/v1/gitops_test.go Outdated Show resolved Hide resolved

rhuss reviewed Dec 8, 2020

View reviewed changes

review comments

0db9aa7

add single file mode

75fad8c

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2020

add single file mode

3fd6e3b

itsmurugappan added 2 commits December 10, 2020 16:31

add single file mode

3b3e575

add single file mode

47f1785

review comments

6721e23

dsimansk reviewed Dec 15, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jan 14, 2021

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

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

knative-prow-robot merged commit f5ac441 into knative:master Jan 14, 2021

dsimansk mentioned this pull request Jan 14, 2021

chore: Add gitops feature to changelog #1190

Merged

rhuss mentioned this pull request Jan 19, 2021

Feature suggestions for offline mode (kn service --target) #1195

Open

itsmurugappan deleted the gitops branch October 11, 2021 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add git ops options #1122

add git ops options #1122

itsmurugappan commented Nov 17, 2020 •

edited

Loading

knative-prow-robot left a comment

maximilien left a comment

maximilien Nov 17, 2020

knative-prow-robot commented Nov 17, 2020

rhuss commented Dec 1, 2020

itsmurugappan commented Dec 7, 2020

rhuss left a comment

rhuss left a comment

rhuss left a comment

itsmurugappan commented Dec 8, 2020

itsmurugappan commented Dec 8, 2020

dsimansk commented Dec 9, 2020

rhuss commented Dec 9, 2020

dsimansk commented Dec 9, 2020

lgtm-com bot commented Dec 11, 2020

itsmurugappan commented Dec 11, 2020

knative-metrics-robot commented Dec 11, 2020

dsimansk Dec 15, 2020

dsimansk Dec 15, 2020 •

edited

Loading

rhuss Jan 14, 2021

rhuss Jan 14, 2021

rhuss commented Dec 15, 2020

rhuss commented Jan 14, 2021

rhuss commented Jan 14, 2021

	flags.String("target", "", "work on local directory instead of a remote cluster")
	flags.String("target", "", "Work on local directory instead of a remote cluster.")

add git ops options #1122

add git ops options #1122

Conversation

itsmurugappan commented Nov 17, 2020 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

maximilien left a comment

Choose a reason for hiding this comment

maximilien Nov 17, 2020

Choose a reason for hiding this comment

knative-prow-robot commented Nov 17, 2020

rhuss commented Dec 1, 2020

itsmurugappan commented Dec 7, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

itsmurugappan commented Dec 8, 2020

itsmurugappan commented Dec 8, 2020

dsimansk commented Dec 9, 2020

rhuss commented Dec 9, 2020

dsimansk commented Dec 9, 2020

lgtm-com bot commented Dec 11, 2020

itsmurugappan commented Dec 11, 2020

knative-metrics-robot commented Dec 11, 2020

dsimansk Dec 15, 2020

Choose a reason for hiding this comment

dsimansk Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

rhuss Jan 14, 2021

Choose a reason for hiding this comment

rhuss Jan 14, 2021

Choose a reason for hiding this comment

rhuss commented Dec 15, 2020

rhuss commented Jan 14, 2021

rhuss commented Jan 14, 2021

itsmurugappan commented Nov 17, 2020 •

edited

Loading

dsimansk Dec 15, 2020 •

edited

Loading