feat: enable full offline lint of all resource types #10059

julienduchesne · 2022-11-18T12:14:32Z

Currently, the offline linting only works for Workflow because other resources (WorkflowTemplate, ClusterWorkflowTemplate and CronWorkflow) can depend on references to other resources.
However, the “online” behavior that fetches refs from the server is also very limiting in a CI context where a user may modify both a "top-level" file and its dependencies at the same time. The dependencies are fetched from the server and the validation fails.

This PR extends the offline linter so that it uses the whole list of files passed as arguments in order to validate. If a ref needs to be resolved, the linter will search through the given files.
This is useful in a GitOps context where a user can pass the whole list of Argo files to the CI check.

Signed-off-by: Julien Duchesne julien.duchesne@grafana.com

Create the PR as draft .
Run make pre-commit -B to fix codegen and lint problems.
Sign-off your commits (otherwise the DCO check will fail).
Use a conventional commit message (otherwise the commit message check will fail).
"Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
Github checks are green.
Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

pkg/apiclient/apiclient.go

alexec · 2022-11-21T15:52:31Z

cmd/argo/commands/lint.go

@@ -35,6 +35,9 @@ func NewLintCommand() *cobra.Command {
  cat manifests.yaml | argo lint --kinds=workflows,cronworkflows -`,
 		Run: func(cmd *cobra.Command, args []string) {
 			client.Offline = offline
+			if offline {
+				client.OfflineFiles = args


I like the idea that all the files form a "linting context", do you even OfflineFiles?

What do you mean?

I think your contextual lint should be the default. No need to enable any flag. I think we can always do what line 39 does. This is always better and more correct.

I removed the condition. It doesn't make sense indeed. The OfflineFiles are only used when the "offline client" is used

cmd/argo/commands/lint.go

cmd/argo/commands/client/conn.go

stale · 2022-12-31T22:23:38Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

stale · 2023-01-21T05:54:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

Currently, the offline linting only works for Workflow because other resources (`WorkflowTemplate`, `ClusterWorkflowTemplate` and `CronWorkflow`) can depend on references to other resources. However, this behavior is very limiting in a CI context where a user may modify both a "top-level" file and its dependencies at the same time. The dependencies are fetched from the server and the validation fails. This PR extends the offline linter so that it uses the whole list of files passed as arguments in order to validate. This is useful in a GitOps context where a user can pass the whole list of Argo files to the CI check. Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

julienduchesne · 2023-02-07T15:40:48Z

Hey @alexec. Any chance I could get another review of this? This would greatly improve performance, reliability and usefulness of the linter for us

alexec

I approve of these changes. @juliev0 do you want to review? If not, we can merge.

juliev0 · 2023-02-12T16:31:25Z

I approve of these changes. @juliev0 do you want to review? If not, we can merge.

My feeling is that in the interest of moving more PRs along, if you find a PR you approve of, go ahead and merge it? (but if it's in an area of the code I've worked in, I'm definitely happy to review it first)

argoproj#10059 was tested in very specific cases it seems because a couple things are wrong: - Linting directories does not work anymore - Resolving references in a namespace which doesn't show up in the given args, panics In this PR, I fix those issues but I also add functional tests that checks that all of these cases work from the cmd level I did not add to the `argo/lint/lint_test.go` tests and instead put the tests earlier down the line because the current tests do not cover the creation and configuration of the offline client Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

julienduchesne changed the title ~~Enable full offline lint of all resources~~ (feat) enable full offline lint of all resources Nov 18, 2022

julienduchesne force-pushed the julienduchesne/offline-scanner branch from 6563cd8 to 8e2f05b Compare November 18, 2022 12:39

julienduchesne changed the title ~~(feat) enable full offline lint of all resources~~ feat: enable full offline lint of all resources Nov 18, 2022

julienduchesne force-pushed the julienduchesne/offline-scanner branch 2 times, most recently from 8973fe4 to 1c6f498 Compare November 18, 2022 13:41

julienduchesne marked this pull request as ready for review November 18, 2022 23:39

alexec requested changes Nov 21, 2022

View reviewed changes

pkg/apiclient/apiclient.go Show resolved Hide resolved

julienduchesne force-pushed the julienduchesne/offline-scanner branch 2 times, most recently from b31c2ec to 3eec7a6 Compare November 21, 2022 15:43

julienduchesne requested a review from alexec November 21, 2022 15:44

alexec reviewed Nov 21, 2022

View reviewed changes

julienduchesne changed the title ~~feat: enable full offline lint of all resources~~ feat: enable full offline lint of all resource types Nov 21, 2022

julienduchesne force-pushed the julienduchesne/offline-scanner branch from 3eec7a6 to 074530a Compare November 21, 2022 16:42

julienduchesne requested a review from alexec November 21, 2022 19:38

alexec reviewed Nov 24, 2022

View reviewed changes

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

cmd/argo/commands/client/conn.go Show resolved Hide resolved

julienduchesne force-pushed the julienduchesne/offline-scanner branch from 074530a to 62a4f49 Compare December 12, 2022 14:44

julienduchesne requested a review from alexec December 12, 2022 14:44

julienduchesne force-pushed the julienduchesne/offline-scanner branch from 62a4f49 to d4336a2 Compare December 16, 2022 19:18

stale bot added problem/stale This has not had a response in some time and removed problem/stale This has not had a response in some time labels Dec 31, 2022

stale bot added problem/stale This has not had a response in some time and removed problem/stale This has not had a response in some time labels Jan 21, 2023

julienduchesne added 2 commits February 7, 2023 10:38

update lint command help

00f7146

Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>

julienduchesne force-pushed the julienduchesne/offline-scanner branch from f9f5c31 to 00f7146 Compare February 7, 2023 15:38

alexec approved these changes Feb 12, 2023

View reviewed changes

alexec added this pull request to the merge queue Feb 13, 2023

alexec removed this pull request from the merge queue due to a manual request Feb 13, 2023

alexec merged commit 43766ca into argoproj:master Feb 13, 2023

julienduchesne mentioned this pull request Feb 21, 2023

fix: Resolve issues with offline linter + add tests #10559

Merged

8 tasks

julienduchesne mentioned this pull request Feb 24, 2023

fix: panic in offline linter + handling stdin #10576

Merged

8 tasks

terrytangyuan mentioned this pull request Aug 2, 2023

Release v3.5 preparation #11381

Closed

This was referenced Sep 4, 2023

Argo lint command that validates any argo object? #4221

Closed

argo lint to handle cron & templates automatically #3730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable full offline lint of all resource types #10059

feat: enable full offline lint of all resource types #10059

julienduchesne commented Nov 18, 2022 •

edited

Loading

alexec Nov 21, 2022

julienduchesne Nov 21, 2022

alexec Nov 21, 2022

julienduchesne Nov 21, 2022

stale bot commented Dec 31, 2022

stale bot commented Jan 21, 2023

julienduchesne commented Feb 7, 2023

alexec left a comment

juliev0 commented Feb 12, 2023

feat: enable full offline lint of all resource types #10059

feat: enable full offline lint of all resource types #10059

Conversation

julienduchesne commented Nov 18, 2022 • edited Loading

alexec Nov 21, 2022

Choose a reason for hiding this comment

julienduchesne Nov 21, 2022

Choose a reason for hiding this comment

alexec Nov 21, 2022

Choose a reason for hiding this comment

julienduchesne Nov 21, 2022

Choose a reason for hiding this comment

stale bot commented Dec 31, 2022

stale bot commented Jan 21, 2023

julienduchesne commented Feb 7, 2023

alexec left a comment

Choose a reason for hiding this comment

juliev0 commented Feb 12, 2023

julienduchesne commented Nov 18, 2022 •

edited

Loading