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: shell scripts #7186

Merged
merged 5 commits into from
Feb 18, 2024
Merged

fix: shell scripts #7186

merged 5 commits into from
Feb 18, 2024

Conversation

martabal
Copy link
Member

@martabal martabal commented Feb 18, 2024

Fix shell scripts which don't respect shellcheck syntax.

Files found with this command : find ./ -type f -name "*.sh" -not \( -path "**/open-api/**" -o -path "**/openapi/**" -o -path "**/node_modules/**" \) -exec shellcheck --format=gcc {} \;

The pump-version.sh script bump the web version too.

@martabal
Copy link
Member Author

I don't know if it's worth adding a CI job for this

install.sh Outdated Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Feb 18, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5f1738d
Status: ✅  Deploy successful!
Preview URL: https://a675b987.immich.pages.dev
Branch Preview URL: https://fix-bash-scripts.immich.pages.dev

View logs

@martabal martabal changed the title fix: bash scripts fix: shell scripts Feb 18, 2024
Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Nice! I think a check would be fine if you'd like to add one

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

So it isn't considered good practice to assign a value to an env variable directly? (without an intermediate variable)

@martabal
Copy link
Member Author

So it isn't considered good practice to assign a value to an env variable directly? (without an intermediate variable)

Yes, if you use export https://www.shellcheck.net/wiki/SC2155

@mertalev
Copy link
Contributor

In the original code, the return value of mycmd is ignored, and export will instead always return true. This may prevent conditionals, set -e and traps from working correctly.

Oh wow, I didn't know it ignored return values.

@alextran1502 alextran1502 merged commit ddae707 into main Feb 18, 2024
26 checks passed
@alextran1502 alextran1502 deleted the fix/bash-scripts branch February 18, 2024 23:03
@martabal martabal mentioned this pull request Feb 18, 2024
claabs pushed a commit to claabs/immich-machine-learning-openvino-kernel-fix that referenced this pull request Aug 11, 2024
* fix: bash scripts

* pr feedback

* wrong variable

* ci: add shellcheck workflow

* fix: missing scripts
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.

5 participants