Skip to content

[NO QA]Add GitHub Action linting#7697

Merged
AndrewGable merged 11 commits into
mainfrom
andrew-actionlint
Feb 15, 2022
Merged

[NO QA]Add GitHub Action linting#7697
AndrewGable merged 11 commits into
mainfrom
andrew-actionlint

Conversation

@AndrewGable

@AndrewGable AndrewGable commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

Details

We have had a few failures and security vulnerabilities reported by this static analyzer, so I've done two things:

  1. Fix all the issues (most were word globbing issues with " " needing to be applied)
  2. Run the linter every commit along with validateActionsAndWorkflows.sh

Fixed Issues

$ #7703

Tests

  1. Merge this PR
  2. Run a full deploy and verify none of the changes have broken anything 😬

@AndrewGable AndrewGable self-assigned this Feb 11, 2022
@AndrewGable AndrewGable changed the title Add GitHub Action linting (Expect lint to *fail*) Add GitHub Action linting Feb 11, 2022
@AndrewGable AndrewGable marked this pull request as ready for review February 11, 2022 19:21
@AndrewGable AndrewGable requested a review from a team as a code owner February 11, 2022 19:21
@MelvinBot MelvinBot requested review from neil-marcellini and removed request for a team February 11, 2022 19:21
Comment thread .github/scripts/validateActionsAndWorkflows.sh Outdated
echo 'Lint Github Actions via actionlint (https://github.com/rhysd/actionlint)'

# If we are running this on a non-CI machine (e.g. locally), install shellcheck
if [[ -z "${CI}" ]]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A bit more robust / faster because it won't run brew install shellcheck every time. Also won't try to do brew install on non-macOS computers.

if [[ -z "${CI}" && -z "$(command -v shellcheck)" ]]; then
  if [[ "$OSTYPE" != 'darwin'* || -z "$(command -v brew)" ]]; then
    echo 'This script requires shellcheck to be installed. Please install it and try again'
    exit 1
  fi

  brew install shellcheck
fi

Comment thread .github/workflows/deployBlocker.yml Outdated
echo "DEPLOY_BLOCKER_URL=${{ github.event.issue.html_url }}" >> $GITHUB_ENV
echo "DEPLOY_BLOCKER_NUMBER=${{ github.event.issue.number }}" >> $GITHUB_ENV
echo "DEPLOY_BLOCKER_TITLE=$(sed -e "s/'/'\\\\''/g; s/\`/\\\\\`/g; 1s/^/'/; \$s/\$/'/" <<< "'${{ github.event.issue.title }}'")" >> $GITHUB_ENV
{ echo "DEPLOY_BLOCKER_URL=${{ github.event.issue.html_url }}"; echo "DEPLOY_BLOCKER_NUMBER=${{ github.event.issue.number }}"; echo "DEPLOY_BLOCKER_TITLE=$(sed -e "s/'/'\\\\''/g; s/\`/\\\\\`/g; 1s/^/'/; \$s/\$/'/" <<< "$TITLE")";} >> "$GITHUB_ENV"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Uhhhh what is this code? It wasn't great before either so NAB but I'm wondering if we could clean this up ... Any idea what that sed is doing? 😬

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At minimum, can we break it into multiple lines w/ \ ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was complaining that I needed to lump the commands together, but I will try new lines with \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well ... still NAB because you didn't introduce this.

@neil-marcellini neil-marcellini left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks pretty good to me as much as I understand it.

@AndrewGable

Copy link
Copy Markdown
Contributor Author

Thanks for the review @roryabraham and @neil-marcellini! I will pick up the review comments later today.

AndrewGable and others added 2 commits February 14, 2022 11:59
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@AndrewGable AndrewGable merged commit b09bd2a into main Feb 15, 2022
@AndrewGable AndrewGable deleted the andrew-actionlint branch February 15, 2022 18:31
@AndrewGable AndrewGable changed the title Add GitHub Action linting [NO QA]Add GitHub Action linting Feb 15, 2022
@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @AndrewGable in version: 1.1.39-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants