Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add argo lint on examples directory #1243

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jeongukjae
Copy link
Collaborator

Pull Request Checklist

Description of PR

This PR adds make lint-argo command to start work on issue #1163
I will create additional fixes after this PR.

+ Since, the actual lint errors are not addressed yet, I didn't add it to CI checks or Contributing docs.

@jeongukjae jeongukjae self-assigned this Oct 17, 2024
@jeongukjae jeongukjae added the type:skip-changelog A change that does not require a changelog entry label Oct 17, 2024
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.9%. Comparing base (12de907) to head (a0c37ed).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1243   +/-   ##
=====================================
  Coverage   45.9%   45.9%           
=====================================
  Files         60      60           
  Lines       4096    4096           
  Branches     861     861           
=====================================
  Hits        1884    1884           
  Misses      2180    2180           
  Partials      32      32           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jeongukjae jeongukjae added semver:patch A change requiring a patch version bump type:task A general task and removed type:skip-changelog A change that does not require a changelog entry labels Oct 17, 2024
@sambhav
Copy link
Collaborator

sambhav commented Oct 21, 2024

@jeongukjae can you add it to the CI now?

@jeongukjae
Copy link
Collaborator Author

@sambhav not yet :( currently I see 55 lint errors if I run make lint-argo in main branch.

@sambhav
Copy link
Collaborator

sambhav commented Oct 21, 2024

@jeongukjae can we run it but not fail on it? Or have some way to freeze the number of errors, so that it fails if it increases?

@jeongukjae
Copy link
Collaborator Author

jeongukjae commented Oct 21, 2024

fyi, there are three kind of lint errors now,

  • minor manifest errors that I can fix (missing entrypoint, parameter, artifacts, ...)
  • errors those need workflow template in the test cluster, and I can also fix, but need some time to set up
  • errors caused by the syntaxes those are introduced in Argo Workflow 3.6 (currently, we use 3.5 in Makefile, so this will be fixed after Argo Workflows 3.6 release changes #1215)

so, how about

  1. fixing first two issues in the following PRs,
  2. and upgrading the Argo Workflow version in hera repo to 3.6
  3. then adding this to CI?

if there's no strong reason to add this now?

through this, I will assign myself to the issue (#1163), and fix this.

cc @elliotgunton

@elliotgunton
Copy link
Collaborator

elliotgunton commented Oct 21, 2024

Yeah, having tried a plain argo lint examples before, I don't think we should try to lint the folder wholesale (due to errors like multiple templates with the same name, plus (workflow)templaterefs).

@jeongukjae
Copy link
Collaborator Author

@elliotgunton makes sense.
How about limiting directories like this? (I added one another commit)

Then remained issues are as follows:

examples/workflows/experimental/new-dag-decorator-artifacts.yaml:
   ✖ in "my-workflow-" (Workflow): rpc error: code = Unknown desc = templates.worker inputs.artifacts.artifact_a was not supplied

examples/workflows/experimental/new-dag-decorator-inner-dag.yaml:
   ✖ in "my-workflow-" (Workflow): rpc error: code = Unknown desc = templates.outer-dag inputs.parameters.value_a was not supplied

examples/workflows/experimental/new-dag-decorator-params.yaml:
   ✖ in "my-workflow-" (Workflow): rpc error: code = Unknown desc = templates.worker inputs.parameters.value_b was not supplied

examples/workflows/experimental/new-decorators-auto-template-refs.yaml:
   ✖ in "my-workflow-" (Workflow): rpc error: code = Unknown desc = templates.worker inputs.parameters.value_b was not supplied

examples/workflows/experimental/new-steps-decorator-with-parallel-steps.yaml:
   ✖ in "my-workflow-" (Workflow): rpc error: code = Unknown desc = templates.worker inputs.parameters.value_b was not supplied

examples/workflows/experimental/script-annotations-artifact-custom-volume.yaml:
   ✖ in "test-output-annotations-" (Workflow): rpc error: code = Unknown desc = templates.my-steps.steps[3].use-artifact-existing-vol templates.use-artifact-existing-vol inputs.artifacts.successor_in was not supplied

examples/workflows/experimental/script-runner-io.yaml:
   ✖ in "pydantic-io-" (Workflow): rpc error: code = Unknown desc = spec.entrypoint is required

examples/workflows/experimental/template-sets.yaml:
   ✖ in "my-workflow" (Workflow): rpc error: code = Unknown desc = spec.entrypoint is required

examples/workflows/steps/steps-types.yaml:
   ✖ in "steps-" (Workflow): rpc error: code = Unknown desc = templates.hello-hello-hello.steps[3].name 'list-of-step-1' is not unique

I think those can be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants