feat(cli): Add offline linting #5569

NikeNano · 2021-04-01T12:47:36Z

Checklist:

My organization is added to USERS.md.

codecov · 2021-04-01T13:00:12Z

Codecov Report

Merging #5569 (a06a556) into master (cc7e310) will decrease coverage by 0.16%.
The diff coverage is 29.21%.

❗ Current head a06a556 differs from pull request most recent head c11df9e. Consider uploading reports for the commit c11df9e to get more accurate results

@@            Coverage Diff             @@
##           master    #5569      +/-   ##
==========================================
- Coverage   47.08%   46.92%   -0.17%     
==========================================
  Files         240      244       +4     
  Lines       15050    15202     +152     
==========================================
+ Hits         7087     7134      +47     
- Misses       7058     7162     +104     
- Partials      905      906       +1

Impacted Files	Coverage Δ
cmd/argo/commands/client/conn.go	`12.76% <0.00%> (-0.57%)`	⬇️
cmd/argo/commands/clustertemplate/create.go	`13.63% <0.00%> (-0.65%)`	⬇️
cmd/argo/commands/clustertemplate/delete.go	`0.00% <0.00%> (ø)`
cmd/argo/commands/clustertemplate/get.go	`0.00% <0.00%> (ø)`
cmd/argo/commands/clustertemplate/lint.go	`0.00% <0.00%> (ø)`
cmd/argo/commands/clustertemplate/list.go	`0.00% <0.00%> (ø)`
cmd/argo/commands/cron/create.go	`0.00% <0.00%> (ø)`
cmd/argo/commands/cron/delete.go	`0.00% <0.00%> (ø)`
cmd/argo/commands/cron/get.go	`24.59% <0.00%> (-0.41%)`	⬇️
cmd/argo/commands/cron/lint.go	`0.00% <0.00%> (ø)`
... and 27 more

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 cc7e310...c11df9e. Read the comment docs.

NikeNano · 2021-04-03T21:21:04Z

@alexec would be great if you could give some pointers on the implementation for offline linting, thanks!

cmd/argo/commands/lint.go

pkg/apiclient/offline-workflow-service-client.go

pkg/apiclient/http1-client.go

cmd/argo/commands/submit.go

cmd/argo/commands/clustertemplate/lint.go

cmd/argo/commands/lint.go

pkg/apiclient/argo-kube-client.go

pkg/apiclient/argo-server-client.go

pkg/apiclient/http1-client.go

pkg/apiclient/offlineclient.go

pkg/apiclient/offline-workflow-service-client.go

cmd/argo/lint/lint.go

alexec · 2021-04-05T19:36:40Z

cmd/argo/commands/client/conn.go

-var explicitPath string
+var (
+ explicitPath string
+ Offline bool


I wonder if we should have ARGO_OFFLINE envvar?

I think it would be better suited as an argument atm since it is only supported for the linting.

alexec · 2021-04-06T20:31:59Z

cmd/argo/commands/cron/create.go

@@ -48,7 +48,10 @@ func NewCreateCommand() *cobra.Command {

 func CreateCronWorkflows(filePaths []string, cliOpts *cliCreateOpts, submitOpts *wfv1.SubmitOpts) {
 ctx, apiClient := client.NewAPIClient()
- serviceClient := apiClient.NewCronWorkflowServiceClient()
+ serviceClient, err := apiClient.NewCronWorkflowServiceClient()
+ if err != nil {


minor: prefer errors.CheckError(err)

I have tried to be consistent with the error handling in each part of the code. Suggest we make a clean up in a seperate PR to errors.CheckError(err). WDYT?

I don't feel strongly. Any comment prefixed with "minor" is a suggestion.

pkg/apiclient/offlineclient.go

Signed-off-by: NikeNano <[email protected]>

alexec · 2021-04-07T22:01:38Z

Lint error

  level=error msg="Running error: buildir: analysis skipped: errors in package: [/home/runner/work/argo-workflows/argo-workflows/test/e2e/cli_test.go:63:7: undeclared name: OFFLINE /home/runner/work/argo-workflows/argo-workflows/test/e2e/cli_test.go:725:12: undeclared name: OFFLINE]"

alexec

Please lint

alexec · 2021-04-07T22:00:26Z

pkg/apiclient/apiclient.go

@@ -62,6 +66,7 @@ func NewClientFromOpts(opts Opts) (context.Context, Client, error) {
 if opts.ClientConfigSupplier != nil {
 opts.ClientConfig = opts.ClientConfigSupplier()
 }
- return newArgoKubeClient(opts.ClientConfig, instanceid.NewService(opts.InstanceID))
+ ctx, client, err := newArgoKubeClient(opts.ClientConfig, instanceid.NewService(opts.InstanceID))
+ return ctx, client, err


suggestion: revert

Do you mean new line? Or that you actually want this to be indented?

alexec · 2021-04-07T22:00:36Z

pkg/apiclient/http1-client.go

@@ -14,6 +14,8 @@ import (

 type httpClient http1.Facade

+var _ Client = &httpClient{}


alexec · 2021-04-07T22:00:59Z

test/e2e/cli_test.go

@@ -719,6 +721,24 @@ func (s *CLISuite) TestWorkflowLint() {
 })
 }

+func (s *CLISuite) TestWorkflowOfflineLint() {


Signed-off-by: NikeNano <[email protected]>

NikeNano · 2021-04-08T19:26:41Z

how do I re-trigger the tests again @alexec?

alexec · 2021-04-08T22:20:23Z

there are some users submitting PRs adding crypto mining code - lets wait for them to go away

NikeNano · 2021-04-12T15:54:57Z

Is is possible to run the tests again @alexec?

NikeNano · 2021-04-12T21:23:10Z

Will continue to work on why the TestArchive fail tomorrow.

Signed-off-by: NikeNano <[email protected]>

… into nikenano/offlineLint

Signed-off-by: NikeNano <[email protected]>

NikeNano · 2021-04-15T19:40:14Z

~~Fixed the tests now @alexec~~

Need to rerun the tests.

Signed-off-by: NikeNano <[email protected]>

NikeNano · 2021-04-16T14:34:16Z

Could you PTALA @alexec , thanks.

alexec · 2021-04-16T15:12:56Z

cmd/argo/commands/lint.go

 },
 }

 command.Flags().StringSliceVar(&lintKinds, "kinds", []string{"all"}, fmt.Sprintf("Which kinds will be linted. Can be: %s", strings.Join(allKinds, "|")))
 command.Flags().StringVarP(&output, "output", "o", "pretty", "Linting results output format. One of: pretty|simple")
 command.Flags().BoolVar(&strict, "strict", true, "Perform strict workflow validation")
+ command.Flags().BoolVar(&offline, "offline", false, "perform offline linting")


suggestion:

command.Flags().BoolVar(&client.Offline, "offline", false, "Go offline - do not connect to server")

NikeNano marked this pull request as draft April 1, 2021 12:48

NikeNano force-pushed the nikenano/offlineLint branch from 13b762a to 494e6ce Compare April 1, 2021 12:54

NikeNano changed the title ~~Add offline linting~~ feat: Add offline linting Apr 1, 2021

NikeNano marked this pull request as ready for review April 3, 2021 21:20

alexec reviewed Apr 4, 2021

View reviewed changes

cmd/argo/commands/lint.go Outdated Show resolved Hide resolved

pkg/apiclient/offline-workflow-service-client.go Show resolved Hide resolved

pkg/apiclient/http1-client.go Outdated Show resolved Hide resolved

alexec changed the title ~~feat: Add offline linting~~ feat(cli): Add offline linting Apr 4, 2021

sarabala1979 self-assigned this Apr 5, 2021

alexec reviewed Apr 5, 2021

View reviewed changes

cmd/argo/commands/submit.go Outdated Show resolved Hide resolved

alexec reviewed Apr 5, 2021

View reviewed changes

alexec reviewed Apr 6, 2021

View reviewed changes

Offline linting for workflows

0e207c2

Signed-off-by: NikeNano <[email protected]>

NikeNano force-pushed the nikenano/offlineLint branch from f979ca7 to 0e207c2 Compare April 7, 2021 20:38

NikeNano added 2 commits April 7, 2021 23:02

fix docs

cd2db4c

Signed-off-by: NikeNano <[email protected]>

added test

9c4c9b9

Signed-off-by: NikeNano <[email protected]>

alexec requested changes Apr 7, 2021

View reviewed changes

lint

4cba9a2

Signed-off-by: NikeNano <[email protected]>

Merge branch 'master' into nikenano/offlineLint

72a52cc

alexec marked this pull request as draft April 13, 2021 18:27

NikeNano added 2 commits April 15, 2021 20:51

updated the test

8a93bd1

Signed-off-by: NikeNano <[email protected]>

Merge branch 'nikenano/offlineLint' of ssh:https://github.com/NikeNano/argo…

2a09922

… into nikenano/offlineLint

NikeNano marked this pull request as ready for review April 15, 2021 19:19

retrigger tests

25e1b72

Signed-off-by: NikeNano <[email protected]>

NikeNano added 2 commits April 15, 2021 21:42

clean up from debugging

1cdbfdd

Signed-off-by: NikeNano <[email protected]>

retrigger

c11df9e

Signed-off-by: NikeNano <[email protected]>

alexec approved these changes Apr 16, 2021

View reviewed changes

alexec added this to the v3.1 milestone Apr 16, 2021

alexec merged commit 8607391 into argoproj:master Apr 16, 2021

simster7 mentioned this pull request Apr 19, 2021

v3.0.2 cherry-pick #5715

Closed

50 tasks

NikeNano mentioned this pull request Jul 6, 2021

chore: add offline linting kubeflow/pipelines#5981

Merged

1 task

paulfouquet mentioned this pull request Apr 15, 2024

ci: move workflows linting to the main job TDE-1143 linz/topo-workflows#529

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): Add offline linting #5569

feat(cli): Add offline linting #5569

NikeNano commented Apr 1, 2021

codecov bot commented Apr 1, 2021 •

edited

Loading

NikeNano commented Apr 3, 2021

alexec Apr 5, 2021

NikeNano Apr 5, 2021

alexec Apr 6, 2021

NikeNano Apr 7, 2021

alexec Apr 16, 2021 •

edited

Loading

alexec commented Apr 7, 2021

alexec left a comment

alexec Apr 7, 2021 •

edited

Loading

NikeNano Apr 8, 2021

alexec Apr 7, 2021

alexec Apr 7, 2021

NikeNano commented Apr 8, 2021

alexec commented Apr 8, 2021

NikeNano commented Apr 12, 2021

NikeNano commented Apr 12, 2021

NikeNano commented Apr 15, 2021 •

edited

Loading

NikeNano commented Apr 16, 2021

alexec Apr 16, 2021

		@@ -14,6 +14,8 @@ import (

		type httpClient http1.Facade

		var _ Client = &httpClient{}

feat(cli): Add offline linting #5569

feat(cli): Add offline linting #5569

Conversation

NikeNano commented Apr 1, 2021

codecov bot commented Apr 1, 2021 • edited Loading

Codecov Report

NikeNano commented Apr 3, 2021

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

alexec Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

alexec commented Apr 7, 2021

alexec left a comment

Choose a reason for hiding this comment

alexec Apr 7, 2021 • edited Loading

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

NikeNano commented Apr 8, 2021

alexec commented Apr 8, 2021

NikeNano commented Apr 12, 2021

NikeNano commented Apr 12, 2021

NikeNano commented Apr 15, 2021 • edited Loading

NikeNano commented Apr 16, 2021

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2021 •

edited

Loading

alexec Apr 16, 2021 •

edited

Loading

alexec Apr 7, 2021 •

edited

Loading

NikeNano commented Apr 15, 2021 •

edited

Loading