Skip to content
New issue

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

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

Already on GitHub? # to your account

Upgrade Shellcheck (fixes #34) #37

Merged
merged 6 commits into from
Oct 29, 2021
Merged

Conversation

Azbagheri
Copy link
Owner

Pull Request Checklist

  • I have created an issue prior to creating this pull request
  • I have provided a detailed description and motivation regarding the change below
  • I have updated the test suite and documentation to support my change

Corresponding Issue

<ADD-ISSUE-HERE>

Description of Change

<ADD-DESCRIPTION-HERE>

Motivation and Context

<ADD-MOTIVATION-HERE>

Testing Steps

<ADD-STEPS-HERE>

Risks

<ADD-RISKS-HERE>

Additional Information

<ADD-ADDITIONAL-INFORMATION-HERE>

@Azbagheri Azbagheri linked an issue Oct 23, 2021 that may be closed by this pull request
4 tasks
@Azbagheri Azbagheri changed the title Upgrade shellcheck (fixes #) Upgrade shellcheck (fixes #34) Oct 23, 2021
@Azbagheri Azbagheri changed the title Upgrade shellcheck (fixes #34) Upgrade Shellcheck (fixes #34) Oct 23, 2021

wget -qO- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv
sudo cp "shellcheck-${scversion}/shellcheck" /usr/bin/
cp "shellcheck-${scversion}/shellcheck" /usr/local/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 💯

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting side-note ~ check out the shellcheck-alpine image. They also seem to remove the shellcheck-${tag} folder

Copy link
Owner Author

Choose a reason for hiding this comment

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

I like the idea of a cleaning up the unnecessary files/folders post installation! Made the change in our script as well!

@@ -1,8 +1,8 @@
#! /bin/bash
#!/bin/bash

# Update shellcheck to the locked v0.7.0 version through the binary distribution
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update this comment? Might be a good practice to indicate why we're updating?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good catch!! I actually removed the comment from the script because in general we shouldn't always have a reason to upgrade. It's a good practice to use the latest stable version. With that said, if we ever need to lock down the shellchck version to something older than the latest version, then we need to leave a comment.

…ck version.

Also upgraded the base image used in the Shell Linter's Dockerfile.
…s it is not managed by the distribution and we won't need to use sudo in the script
…n by removing the shellcheck folder after the shellcheck executable is added to the path
@Azbagheri Azbagheri force-pushed the feature/upgrade-shellcheck branch from 7f71087 to e498b4f Compare October 28, 2021 01:41
@asadmansr asadmansr self-requested a review October 29, 2021 00:31
@Azbagheri Azbagheri merged commit 721aa46 into develop Oct 29, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore doesn't work correctly
2 participants