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

fix: panic in offline linter + handling stdin #10576

Merged

Conversation

julienduchesne
Copy link
Contributor

@julienduchesne julienduchesne commented Feb 24, 2023

Follow-up to #10059 and #10559

The offline linter is working well in ideal scenarios, I am linting a set of files/directory in 5 seconds where it used to take ~5 minutes

However, I did find issues after testing some more:
Given an non-existent file, it panics (I inverted the info.IsDir() statement and the one checking for errors)
There's a disconnect between the filepath.Walk in the linter and the one building the offline client which means that there were other issues (such as stdin not working)

To fix those issues once and for all, I extracted the walk func to a common path (file.WalkManifests); that way, we are sure that changes are always reflected in both locations

I also added some tests for non-existent files as well as offline linting from stdin

  • 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".

@julienduchesne julienduchesne force-pushed the julienduchesne/fix-possible-panic branch from 0129d21 to a85e99b Compare February 24, 2023 03:11
@julienduchesne julienduchesne marked this pull request as ready for review February 24, 2023 03:18
@julienduchesne julienduchesne force-pushed the julienduchesne/fix-possible-panic branch 2 times, most recently from 1916e68 to 30dd400 Compare February 24, 2023 14:41
The offline linter is working well in ideal scenarios, but given an non-existent file, it panics (I inverted the `info.IsDir()` statement and the one checking for errors)
There's a disconnect between the filepath.Walk in the linter and the one building the offline client which means that there were other issues (such as stdin not working)

To fix that once and for all, I extracted the walk func to a common path (file.WalkManifests); that way, we are sure that changes are always reflected in both locations

I also added some tests for non-existent files as well as offline linting from stdin

Signed-off-by: Julien Duchesne <julien.duchesne@grafana.com>
@julienduchesne julienduchesne force-pushed the julienduchesne/fix-possible-panic branch from 30dd400 to 86b8b19 Compare February 24, 2023 14:42
@alexec alexec merged commit 51ed115 into argoproj:master Feb 26, 2023
@tooptoop4
Copy link
Contributor

@julienduchesne is there an argo binary anywhere with this fix? or do I need to compile the binary?

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.

3 participants