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

feat: added lint from stdin #5095

Merged
merged 13 commits into from Feb 19, 2021
Merged

feat: added lint from stdin #5095

merged 13 commits into from Feb 19, 2021

Conversation

rkrmr33
Copy link
Contributor

@rkrmr33 rkrmr33 commented Feb 12, 2021

Checklist:

Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
@rkrmr33 rkrmr33 changed the title added lint from stdin feat: added lint from stdin Feb 12, 2021
@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 12, 2021

feature preview

  • new lint format:

    image
  • lint all argo manifests types with the --all-kinds flag:

    image
  • can change the lint output format, either simple or pretty (default):

    image
  • success message:

    image

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only gave this a quick pass, but looks great so far! Will give a more detailed look later

workflow/common/parse.go Outdated Show resolved Hide resolved
workflow/lint/lint.go Outdated Show resolved Hide resolved
workflow/lint/lint.go Outdated Show resolved Hide resolved
cmd/argo/commands/template/lint.go Outdated Show resolved Hide resolved
@alexec
Copy link
Contributor

alexec commented Feb 12, 2021

Look great! Rather than say “error in object #1” can we say “error in WorkflowTemplate named foo”?

@simster7 simster7 self-assigned this Feb 12, 2021
@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 13, 2021

Look great! Rather than say “error in object #1” can we save “error in WorkflowTemplate named foo”?

Hmm, the thing is, the objects don't always have a name to recognize them by, for example:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: workflow-template-hello-world-
  labels:
    argo-e2e: "true"
spec:
  entrypoint: whalesay-template
  arguments:
    parameters:
      - name: message
        value: "hello world"

This doesn't have a name rather a generateName. WDYT we should do in that case?

rkrmr33 and others added 2 commits February 13, 2021 13:39
Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 16, 2021

I've changed it so that it shows the object's name and type instead of index. If the object has a generateName, I just show the generateName instead.

image

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, just some more minor changes

test/e2e/cron/basic.yaml Outdated Show resolved Hide resolved
workflow/lint/lint.go Outdated Show resolved Hide resolved
@alexec
Copy link
Contributor

alexec commented Feb 16, 2021

Look great! Rather than say “error in object #1” can we save “error in WorkflowTemplate named foo”?

Hmm, the thing is, the objects don't always have a name to recognize them by, for example:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: workflow-template-hello-world-
  labels:
    argo-e2e: "true"
spec:
  entrypoint: whalesay-template
  arguments:
    parameters:
      - name: message
        value: "hello world"

This doesn't have a name rather a generateName. WDYT we should do in that case?

I think they must have either name or generateName. Could you use one or the other?

I think this is fantastic BTW. I just want to give argo lint a directory of files and it lints the lot, pods, services, workflows, cronworkflows, everything!

rkrmr33 and others added 4 commits February 16, 2021 19:06
…nto lint-from-stdin

Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
…nto lint-from-stdin

Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
…nto lint-from-stdin

Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
Signed-off-by: arckey <roi.kramer@codefresh.io>
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s a go library called go-color we should adopt for colourising terminal output.

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @alexec Any final comments?

@simster7
Copy link
Member

@arckey Please make sure you make the DCO check happy. It seems that some commits are not signed: https://github.com/argoproj/argo-workflows/pull/5095/checks?check_run_id=1916767542

@alexec
Copy link
Contributor

alexec commented Feb 17, 2021

Questions:

  • Have we taken this opportunity to unify the lining into a single command that can lint multiple file types?
  • What happens with DUMB terminals? Can we use the go-color library to mitigate problems?
  • Do we print lining errors as we encounter them? On just at the end? What happens with many files?

I don' think we need --all-kinds, I think this actually the case today. I think we should always try to lint everything we're given

I think --format should be --output/-o maybe?

Should we add argo lint examples/* to make sure all our examples lint?

Signed-off-by: arckey <roikramer120@gmail.com>
@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 18, 2021

@arckey Please make sure you make the DCO check happy. It seems that some commits are not signed: https://github.com/argoproj/argo-workflows/pull/5095/checks?check_run_id=1916767542

sorry, did not have time to finish this today, will close everything tomorrow :)

@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 18, 2021

Questions:

  • Have we taken this opportunity to unify the lining into a single command that can lint multiple file types?
  • What happens with DUMB terminals? Can we use the go-color library to mitigate problems?
  • Do we print lining errors as we encounter them? On just at the end? What happens with many files?

I don' think we need --all-kinds, I think this actually the case today. I think we should always try to lint everything we're given

I think --format should be --output/-o maybe?

Should we add argo lint examples/* to make sure all our examples lint?

  1. Yes, we can now lint all of argo workflows' types with a single command, do you think that --all-kinds should be the default for the argo lint command? do we still want to allow users to lint only Workflow like they could before?
  2. Sure, I can change it to --output/-o, makes sense.
  3. Do you mean we should add some e2e test to check that?

about the above points:

  1. Yes
  2. I was not able to find go-color library, I know that one: https://github.com/fatih/color is pretty popular. And it has a --no-color flag. Do you think we should use it?
  3. We aggregate all of the results from all of the files/stdin and then we process them and print a summary.

@alexec
Copy link
Contributor

alexec commented Feb 18, 2021

  1. I think users just want to lint. So it'd be great if argo lint linted everything. argo template lint etc could just aliases to that command.
  2. Forget about e2e tests. Avoid them if we can.
  3. I just merged a PR to master than adds go-color to go.mod. See test/e2e/fixtures/print.go for example of usage.
  4. I think it would be really valuable if we print errors as we encounter them. As a user, I might cancel the lint as soon as a see an error. If I'm linting a large directory, then it could take a while and I don't want to wait to see errors.

@alexec
Copy link
Contributor

alexec commented Feb 18, 2021

Oh argo-e2e has been renamed to workflows.argoproj.io/test

@alexec
Copy link
Contributor

alexec commented Feb 18, 2021

Oh, as e2e test automatically add it, you can probably remove it.

@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 18, 2021

  • I think users just want to lint. So it'd be great if argo lint linted everything. argo template lint etc could just aliases to that command.
  • Forget about e2e tests. Avoid them if we can.
  • I just merged a PR to master than adds go-color to go.mod. See test/e2e/fixtures/print.go for example of usage.
  • I think it would be really valuable if we print errors as we encounter them. As a user, I might cancel the lint as soon as a see an error. If I'm linting a large directory, then it could take a while and I don't want to wait to see errors.
  1. Makes sense, will change.
  2. So what did you mean with:

Should we add argo lint examples/* to make sure all our examples lint?

  1. Great! will use that.
  2. Sure, I can see what it takes to do that. Don't think it would be that difficult.

@alexec
Copy link
Contributor

alexec commented Feb 18, 2021

Should we add argo lint examples/* to make sure all our examples lint?

Examples should lint. How about adding argo lint examples/* to the make lint target?

Signed-off-by: roi.kramer <roi.kramer@codefresh.io>
@alexec alexec merged commit 377c5f8 into argoproj:master Feb 19, 2021
@alexec
Copy link
Contributor

alexec commented Feb 19, 2021

Lets do this!

@rkrmr33
Copy link
Contributor Author

rkrmr33 commented Feb 19, 2021

@alexec Haven't made the last changes we discussed about, but I guess I can do that with a follow up PR 😉

@rkrmr33 rkrmr33 deleted the lint-from-stdin branch February 19, 2021 18:03
@simster7 simster7 mentioned this pull request Feb 23, 2021
34 tasks
@rkrmr33 rkrmr33 mentioned this pull request Feb 26, 2021
1 task
@simster7 simster7 mentioned this pull request Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants