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

Apply shellcheck to vscodium root directory scripts #1569

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Apply shellcheck to vscodium root directory scripts #1569

merged 3 commits into from
Jul 20, 2023

Conversation

zpiatt
Copy link
Contributor

@zpiatt zpiatt commented Jul 20, 2023

All shell scripts within the VSCodium repository's root directory now pass shellcheck.

  1. Declared all variables separate from assignment (https://www.shellcheck.net/wiki/SC2155 .
  2. Fixed all instances of reading/writing files in same pipeline (https://www.shellcheck.net/wiki/SC2094).
  3. Quoted all variables (https://www.shellcheck.net/wiki/SC2086).
  4. Double quoted expressions to be expanded (https://www.shellcheck.net/wiki/SC2016)
    • NOTE: this required escape characters in some cases.
  5. Removed legacy back-ticks (https://www.shellcheck.net/wiki/SC2006).
  6. Removed $ and {} from arithmetic variables (https://www.shellcheck.net/wiki/SC2004).
  7. Removed useless cat & echo (https://www.shellcheck.net/wiki/SC2002).
  8. Added -r to read commands (https://www.shellcheck.net/wiki/SC2162).
  9. Converted consecutive individual redirects to single group redirects (style) (https://www.shellcheck.net/wiki/SC2129).
  10. Replaced ! -z with -n and vice-versa (style) (https://www.shellcheck.net/wiki/SC2236).

Special shellcheck considerations:

  1. shellcheck notes when it is unable to follow variables or files when sourced. When applicable, I ignored this message (https://www.shellcheck.net/wiki/SC1091) with a directive (https://github.com/koalaman/shellcheck/wiki/directive).
  2. shellcheck notes when a variable is assigned but not referenced within current the script. I found a single case, however I believe the variable is being utilized elsewhere in the build process so I ignored this message (https://www.shellcheck.net/wiki/SC2154).

Other small changes:

  1. I standardized all shebangs with #!/usr/bin/env bash.
  2. I standardized all tests with [[ ]].

There's a lot here. If you need me to be more thorough, I can provide line-by-line explanations. If this looks good to you, I can continue this on the sub-directories. Also, apologies for the accidental commit on my last PR after workflows ran.

Copy link
Member

@daiyam daiyam left a comment

Choose a reason for hiding this comment

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

@zpiatt Thx for the PR.

I found a single case, however I believe the variable is being utilized elsewhere in the build process so I ignored this message

Which variable was it?

check_cron_or_pr.sh Outdated Show resolved Hide resolved
check_tags.sh Outdated Show resolved Hide resolved
get_repo.sh Outdated Show resolved Hide resolved
install_gh.sh Outdated Show resolved Hide resolved
prepare_vscode.sh Outdated Show resolved Hide resolved
update_insider.sh Outdated Show resolved Hide resolved
update_version.sh Outdated Show resolved Hide resolved
update_version.sh Outdated Show resolved Hide resolved
update_version.sh Outdated Show resolved Hide resolved
version.sh Outdated Show resolved Hide resolved
@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 20, 2023

"${npm_config_arch}" lines 63 & 65 of prepare_vscode.sh. I found it referenced in several places:
.github/workflows/stable-windows.yml: line 82
.github/workflows/insider-linux.yml: line 97
.github/workflows/insider-linux.yml: line 187
.github/workflows/stable-linux.yml: line 94
.github/workflows/stable-linux.yml: line 182
.github/workflows/insider-windows.yml: line 86

@daiyam
Copy link
Member

daiyam commented Jul 20, 2023

Yep, npm_config_arch is from the workflows

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 20, 2023

All the changes you requested are complete. All scripts continue to pass shellcheck. The only outstanding question I had was whether you were satisfied with my temp variable solution to prevent reading/writing from the same pipeline:

jsonTmp=$(jq --arg 'tag' "${MS_TAG/\-insider/}" --arg 'commit' "${MS_COMMIT}" '. "insider.json" | .tag=$tag | .commit=$commit')
echo "$jsonTmp" > "insider.json"

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 20, 2023

All the changes you requested are complete. All scripts continue to pass shellcheck. The only outstanding question I had was whether you were satisfied with my temp variable solution to prevent reading/writing from the same pipeline:

jsonTmp=$(jq --arg 'tag' "${MS_TAG/\-insider/}" --arg 'commit' "${MS_COMMIT}" '. "insider.json" | .tag=$tag | .commit=$commit')
echo "$jsonTmp" > "insider.json"

I realized as I posted this, I missed unsetting the variable here.

prepare_vscode.sh Outdated Show resolved Hide resolved
prepare_vscode.sh Outdated Show resolved Hide resolved
prepare_vscode.sh Outdated Show resolved Hide resolved
update_version.sh Outdated Show resolved Hide resolved
@daiyam
Copy link
Member

daiyam commented Jul 20, 2023

It look for me. Do you want to continue in the sub-folders?

@zpiatt
Copy link
Contributor Author

zpiatt commented Jul 20, 2023

It look for me. Do you want to continue in the sub-folders?

I was planning to do a separate PR for the rest of the shell scripts as this took me a few hours to get done. If you'd prefer I can try to knock those out as apart of this, but it won't be right away.
Let me know!

@daiyam daiyam merged commit b4318d7 into VSCodium:master Jul 20, 2023
15 checks passed
@daiyam
Copy link
Member

daiyam commented Jul 20, 2023

I did spent some time to make those jq one-liner expression. Oh well, if it's simpler to read with temp variables, I'm ok with it. 😉

Anyways, @zpiatt thank you for this PR and your feedbacks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants