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 new GH shellcheck action #305

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

Conversation

leo8a
Copy link
Contributor

@leo8a leo8a commented May 26, 2022

This PR adds a GH action that checks errors in shell scripts for every commit to main. As a complement, fixes to pass the shellcheck linter can be found in #397, #398, #399, #400, and #401.

Signed-off-by: Leo Ochoa [email protected]

@leo8a
Copy link
Contributor Author

leo8a commented May 26, 2022

/cc @alknopfler

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for making this first PR

@fbac
Copy link
Contributor

fbac commented Jun 9, 2022

Triggering the test-ci manually, we'll merge the PR once the test has passed.

Copy link
Contributor

@fbac fbac left a comment

Choose a reason for hiding this comment

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

LGTM.

Checks triggered.

@flaper87
Copy link
Contributor

flaper87 commented Jun 9, 2022

I have updated the branch just to make sure that all the latest changes pass the checks too.

/lgtm

Copy link
Contributor

@iranzo iranzo left a comment

Choose a reason for hiding this comment

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

@leo8a sorry for the delay, but the pipe was not in shape for doing the testing.

We realized that with this PR in place, it breaks apparently the copy of some files and hence, the installation fails:

image

This works fine on main branch.

I would suggest to split this into different PR's:

  • One for the GHA for the checks
  • One PR for each of the file changes, in that way, we run the pipe to see if it gets affected on that specific file and change without having to debug or rework on it as a whole

What do you think?

@fbac fbac added the Hold Dont merge this yet! label Jun 13, 2022
@leo8a
Copy link
Contributor Author

leo8a commented Jun 14, 2022

Hey team, sure, no problem on my side.

You'll find fixes to pass the shellcheck linter on #397, #398, #399, #400, and #401. Once those are in place, I'll just update this to get in the GH action.

Cheers!

@leo8a leo8a changed the title Add shellcheck and fix scripts to pass the linter Add new GH shellcheck action Jun 14, 2022
@fbac
Copy link
Contributor

fbac commented Jun 14, 2022

Thanks @leo8a! Rebasing the PR branch and testing again.

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@iranzo iranzo removed the test-ci label Jun 14, 2022
This GH action checks (in every commit to main) the shell scripts. Will only flag `severity: error` level problems as failures. Feel free to add `warnings` as error too if you need so.
Copy link
Contributor

@iranzo iranzo left a comment

Choose a reason for hiding this comment

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

LGTM for me, let's try to get the other stuff in and merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants