chore(scripts): Stricter shell script checking #16242
Conversation
WalkthroughThe pull request modifies several scripts and the GitHub Actions workflow configuration in a monorepo setup. Key changes include updating the ShellCheck action in the workflow, enhancing variable quoting in various scripts to improve robustness, and replacing a Podman script call with a Docker script call. Additionally, a previously existing script for Podman has been deleted. The overall structure and logic of the scripts remain largely unchanged, focusing primarily on improving error handling and variable management. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16242 +/- ##
=======================================
Coverage 36.86% 36.87%
=======================================
Files 6798 6796 -2
Lines 140499 140468 -31
Branches 39946 39930 -16
=======================================
Hits 51795 51795
+ Misses 88704 88673 -31
Flags with carried forward coverage won't be shown. Click here to find out more. see 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 70 Total Test Services: 0 Failed, 68 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
scripts/stop-test-proxies.sh (2)
Line range hint
1-21: Consider enhancing error handling and user feedbackThe script has a good structure and follows several best practices. However, consider the following improvements:
- Add error checking after the
${builder} container rmcommand to ensure it succeeded.- Provide more detailed feedback to the user, such as whether each proxy was successfully stopped or if there was an error.
- Consider adding a check to verify if the specified builder (docker or custom) is available before attempting to use it.
These enhancements would improve the script's robustness and user-friendliness.
Line range hint
1-21: Good adherence to guidelines, consider NX integration if applicableThe script adheres well to the specified guidelines:
- It's efficient, readable, and maintainable.
- It uses an environmental variable (${builder}) for flexibility.
However, there's no apparent integration with NX workspace utilities and commands. If this script is part of a larger NX workspace:
- Consider whether it could benefit from integration with NX commands or utilities.
- If applicable, you might want to explore using NX's script running capabilities or integrating this script into the NX workflow.
If NX integration is not applicable for this specific script, please disregard this suggestion.
scripts/ci/generate-build-chunks.sh (1)
Line range hint
1-18: Script adheres well to coding guidelines.The script meets the requirements for efficiency, readability, and maintainability. It uses environmental variables (e.g., PROJECT_ROOT) and integrates well with NX workspace utilities (generate-chunks.sh).
For consistency, consider quoting the
$@in the for loop:for target in "$@"; doThis maintains the pattern of careful variable handling established by your recent changes.
scripts/ci/50_upload-coverage.sh (1)
Line range hint
1-26: Overall script analysis and minor suggestionThe script adheres well to the coding guidelines:
- It's efficient and focused on a single task (uploading coverage reports).
- It uses environmental variables (APP, PROJECT_ROOT, CODECOV_TOKEN) for flexibility.
- It integrates with NX workspace utilities and commands.
- The script is readable and maintainable, with good error handling.
One minor suggestion for improvement:
Consider adding a check for the CODECOV_TOKEN variable at the beginning of the script to ensure it's set before proceeding. This could prevent potential issues if the token is missing. For example:
if [ -z "$CODECOV_TOKEN" ]; then echo "Error: CODECOV_TOKEN is not set" exit 1 fiscripts/ci/list-unaffected.sh (1)
Line range hint
1-38: Consider using a configuration file for improved flexibilityWhile the script adheres well to most of the coding guidelines, including the use of environmental variables and integration with NX workspace utilities, it could benefit from increased flexibility. Consider extracting configurable values (such as the
AFFECTED_ALLprefix "7913-") into a separate configuration file. This would allow easier customization without modifying the script itself, enhancing maintainability and adaptability to different environments.Example:
- Create a
config.shfile:# config.sh AFFECTED_ALL_PREFIX="7913-"
- Source this file at the beginning of the script:
source "$DIR/config.sh"
- Use the configuration variable:
ALL=$(AFFECTED_ALL=${AFFECTED_ALL_PREFIX}${BRANCH} "$DIR"/_nx-affected-targets.sh "$TARGET" | tr -d '\n')This change would align even more closely with the guideline for "Usage of environmental variables and configuration files for flexibility."
scripts/ci/00_prepare-base-tags.sh (1)
13-13: Improved environment variable handlingThe addition of quotes around
$GITHUB_ENVis a good practice. It ensures correct handling of the environment variable, even if it contains spaces or special characters.For consistency and to further improve robustness, consider also quoting
$NX_AFFECTED_ALL:- echo "NX_AFFECTED_ALL=$NX_AFFECTED_ALL" >>"$GITHUB_ENV" + echo "NX_AFFECTED_ALL=\"$NX_AFFECTED_ALL\"" >>"$GITHUB_ENV"This change would ensure that the value of
NX_AFFECTED_ALLis treated as a single argument, even if it contains spaces or special characters.scripts/ci/_docker.sh (3)
Line range hint
1-76: Good use of environment variables, with room for improvementThe script effectively uses environment variables for configuration, providing flexibility. It sets default values for variables like
CONTAINER_BUILDERandDOCKER_LOCAL_CACHE, which is a good practice.To further improve configuration management, consider:
- Using a separate configuration file (e.g.,
.env) for default values, making it easier to manage and update configurations across different environments.- Documenting the purpose and expected values of each environment variable used in the script.
Would you like assistance in implementing these improvements?
10-12: Good integration with NX workspace, with room for improvementThe script effectively integrates with NX workspace utilities by using
yarn nx show projectto retrieve project information. The use ofjqfor parsing the JSON output is a good practice.To enhance robustness, consider adding error handling for cases where the NX command might fail or return unexpected results. For example:
APP_HOME=$(yarn nx show project "$APP" | jq ".root" -r) if [ -z "$APP_HOME" ]; then echo "Error: Failed to retrieve project root for $APP" >&2 exit 1 fiThis addition would help catch and report issues related to NX workspace integration.
Would you like assistance in implementing this error handling improvement?
Line range hint
1-76: Suggestions for further improvementsWhile the script is generally well-structured, consider the following improvements:
Consistent variable naming: Standardize on either uppercase or lowercase for variable names. Typically, uppercase is used for environment variables and constants, while lowercase is used for local variables.
Variable declarations: Consider explicitly declaring all variables at the beginning of the script. This practice improves readability and helps prevent issues with undeclared variables.
Usage/Help section: Add a function that displays usage information and available options. This can be triggered with a
-hor--helpflag, making the script more user-friendly.Example implementation:
usage() { echo "Usage: $0 [DOCKERFILE] [TARGET] [ACTION]" echo "Options:" echo " DOCKERFILE: Path to Dockerfile (default: Dockerfile)" echo " TARGET: Build target (e.g., output-local, output-jest)" echo " ACTION: Action to perform (default: docker_build)" exit 1 } # Add this at the beginning of the main() function if [[ "${1:-}" == "-h" || "${1:-}" == "--help" ]]; then usage fiWould you like assistance in implementing these improvements?
scripts/local-e2e.sh (1)
Line range hint
1-279: Suggestions for improving script structure and robustnessWhile the script is generally well-structured, consider the following improvements:
- Use functions for common tasks to improve readability and maintainability.
- Consistently quote variables to prevent issues with spaces in values.
- Replace
echowithprintffor more robust output handling.Here's an example of how you could refactor a part of the script:
- echo "Usage: build-container.sh [OPTIONS]" - echo "Options:" - echo " -t, --target Set the target build stage" - echo " -f, --dockerfile Specify the Dockerfile to use (default: Dockerfile)" - echo " -p, --publish Publish the image to a registry with the specified tag" - echo " -h, --help Show this help message" + print_usage() { + printf "Usage: %s [OPTIONS]\n" "$(basename "$0")" + printf "Options:\n" + printf " -t, --target Set the target build stage\n" + printf " -f, --dockerfile Specify the Dockerfile to use (default: Dockerfile)\n" + printf " -p, --publish Publish the image to a registry with the specified tag\n" + printf " -h, --help Show this help message\n" + } + + print_usageThis refactoring improves readability and makes the usage message easier to maintain.
.github/workflows/pullrequest.yml (1)
287-294: Improved ShellCheck integration with better configurationThe update to
reviewdog/action-shellcheck@v1is a good improvement. It provides better integration with GitHub and more detailed feedback. The new configuration parameters enhance the effectiveness of the ShellCheck process.Consider adding a
shellcheck_flagsparameter to customize ShellCheck behavior if needed. For example:shellcheck_flags: -e SC1090 -e SC1091This would allow you to exclude specific ShellCheck rules if they're not applicable to your project.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- .github/workflows/pullrequest.yml (1 hunks)
- infra/scripts/helm-diff.sh (1 hunks)
- scripts/ci/00_prepare-base-tags.sh (1 hunks)
- scripts/ci/50_upload-coverage.sh (2 hunks)
- scripts/ci/_docker.sh (1 hunks)
- scripts/ci/_podman.sh (0 hunks)
- scripts/ci/generate-build-chunks.sh (1 hunks)
- scripts/ci/list-unaffected.sh (1 hunks)
- scripts/local-e2e.sh (1 hunks)
- scripts/stop-test-proxies.sh (1 hunks)
💤 Files with no reviewable changes (1)
- scripts/ci/_podman.sh
🧰 Additional context used
📓 Path-based instructions (7)
scripts/ci/00_prepare-base-tags.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
scripts/ci/50_upload-coverage.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
scripts/ci/_docker.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
scripts/ci/generate-build-chunks.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
scripts/ci/list-unaffected.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
scripts/local-e2e.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
scripts/stop-test-proxies.sh (1)
Pattern
scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (12)
scripts/stop-test-proxies.sh (1)
10-10: Improved variable quoting enhances script robustnessThe addition of quotes around
"${proxy}"in the command substitution is a good practice. This change ensures that the script can handle proxy names containing spaces or special characters without breaking. It improves the overall reliability of the script without changing its functionality.scripts/ci/generate-build-chunks.sh (3)
Line range hint
1-18: Overall script structure and functionality look good.The script follows good practices for shell scripting, including:
- Proper shebang and strict error handling
- Sourcing common functionality
- Efficient use of jq for JSON manipulation
- Integration with other scripts in the CI pipeline
This aligns well with the coding guidelines for script efficiency, readability, and maintainability.
10-10: Good improvement in variable handling.Enclosing
$targetin double quotes when passing it togenerate-chunks.shis a positive change. This practice prevents word splitting and globbing, making the script more robust when handling target names with spaces or special characters.
12-12: Improved JSON construction with proper quoting.The addition of double quotes around
$targetin the jq command is a valuable improvement. This ensures that thedocker_typefield in the resulting JSON is always a valid string, regardless of the contents of$target. This change prevents potential JSON parsing errors and maintains data integrity.scripts/ci/50_upload-coverage.sh (2)
9-9: Improved variable quoting for robustnessThe addition of quotes around
"$APP"in the command substitution is a good practice. This change enhances the script's robustness by correctly handling potential spaces or special characters in the$APPvariable, preventing word splitting issues.
19-19: Enhanced input redirection with proper quotingThe addition of quotes around
"$COVERAGE_FILE"in the input redirection is a good improvement. This change ensures that the script correctly handles filenames containing spaces or special characters, preventing potential issues with file reading.infra/scripts/helm-diff.sh (1)
25-26: Improved variable interpolation in curl commands.The change from single quotes to double quotes in the curl commands is correct and necessary. This modification allows for proper expansion of the ${1} and ${2} variables, ensuring that the script correctly uses the provided Git references when fetching the YAML content.
scripts/ci/list-unaffected.sh (1)
12-13: Improved variable handling enhances script robustnessThe addition of quotes around
"$TARGET"and"$DIR"in the calls to_nx-affected-targets.shis a positive change. This modification enhances the script's robustness by ensuring proper handling of variables that may contain spaces or special characters. It aligns well with best practices for shell scripting and improves the overall maintainability of the code.scripts/ci/00_prepare-base-tags.sh (2)
10-10: Improved path handling for main.jsThe addition of quotes around the path to
main.jsis a good practice. It enhances the script's robustness by preventing potential issues when paths contain spaces or special characters.
Line range hint
1-38: Overall script quality assessmentThe script adheres well to the provided coding guidelines:
- It demonstrates good efficiency and maintainability, with recent changes further improving these aspects.
- It effectively uses environmental variables (e.g.,
GITHUB_ENV,NX_AFFECTED_ALL) for flexibility.- The script integrates with NX workspace utilities by setting
NX_AFFECTED_ALL.The recent changes have enhanced the script's robustness in handling file paths and environment variables, which is a positive improvement.
scripts/ci/_docker.sh (2)
10-10: Improved variable handling: ApprovedThe addition of double quotes around
"$APP"in the command that setsAPP_HOMEis a good practice. This change enhances the script's robustness by ensuring proper handling of the$APPvariable, especially if it contains spaces or special characters. The modification aligns with shell scripting best practices and maintains consistency with the quoting style used throughout the rest of the script.
Line range hint
1-76: Well-structured and maintainable script: ApprovedThe script demonstrates good practices for efficiency, readability, and maintainability:
- Logical organization using functions
- Robust error handling with
set -euo pipefail- Debug mode for easier troubleshooting
- Modular design by sourcing common functionality
These practices contribute to a more maintainable and efficient codebase.
* Set style level for shellcheck * Linting & formatting scripts * Remove _podman.sh script * Format all scripts * Add reviewdog/action-shfmt step * Configure shfmt * Merge from main * Linting * Move shfmt to before lint * Remove reviewdog * Allow external sources in shellcheck * Use Reviewdog for shellcheck * Set version for Reviewdog --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…-pages (#16234) * Service portal removal. Add portals my pages * minor fixes * Fix * path fix * fix(portals-admin): locklist (#16279) * fix(portals-admin): locklist * tweak * msg id fix * tweak --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(service-portal): feature flag resolver for documents (#16285) * fix: def info and alert * feat: add feature flag to resolver * fix: move ff call to seperate function --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(vehicles-bulk-mileage): Fixes after testing review (#16295) * fix: testing fixes v1 * fix: testing comments v2 * fix: better message * fix: function name * fix: duplicate loading --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(tests): New @island/testing/e2e library (#16287) * Add @swc-node/register and @swc/core * Add testing/e2e library * update project.json for testing/e2e * fix import for libTestingE2e --------- Co-authored-by: Kristofer <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(parental-leave): ApplicationRights (#15901) * feat(parental-leave): ApplicationRights Added applicationRights to parental-leave when sending application. Since we are using a new way of calculating periods * Fix days used by period calculation * Tests for new periods * rename function with proper camelCase * Refactor: Made duplicate code into a function * Make ApplicationRights nullable * refactor: function instead of duplicate code * remove console.log * error handling for period data * clientConfig nullable fix * Fixes for calculation of months. And using clamp to get correct value of daysLeft * Multiply amount of months by 30 for period calculation with month durations * Fix old calculation of endDate with months --------- Co-authored-by: hfhelgason <[email protected]> Co-authored-by: veronikasif <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(passport-application): Updated readme (#16296) * updated readme * updated readme * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(regulations-admin): date format signature, remove self affect, disclaimer text (#16288) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(regulations-admin): No diff no addition in appendix (#16293) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(web): Global alert banner - Handle null case (#16298) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(web): Change custom syslumenn pages config for header (#16299) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(j-s): Digital mailbox API (#16301) * feat(j-s): Block create subpoena on staging and dev * Update subpoena.service.ts * fix(j-s): Fix mailbox API * remove changes not meant for this branch * Update subpoena.service.ts * fix(j-s): reverting changes from other branch * Update subpoena.response.ts * Update subpoena.response.ts * Update subpoena.response.ts * Update subpoena.response.ts --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collection): Fix list reviewed toggle (#16300) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * chore(scripts): Stricter shell script checking (#16242) * Set style level for shellcheck * Linting & formatting scripts * Remove _podman.sh script * Format all scripts * Add reviewdog/action-shfmt step * Configure shfmt * Merge from main * Linting * Move shfmt to before lint * Remove reviewdog * Allow external sources in shellcheck * Use Reviewdog for shellcheck * Set version for Reviewdog --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * chore(new-primary-school): Update messages namespace (#16302) Co-authored-by: veronikasif <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(driving-license): check if 65+ renewal is possible (#16292) * check if 65 renewal is possible * remove console log * cleanup * coderabbit tweaks * coderabbit changes * quick fix * add type? --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(service-portal): default defender and has chosen fields for subpoena (#16306) * fix: def info and alert * feat: add feature flag to resolver * fix: move ff call to seperate function * feat: add default choices ans has chosen + loading states * fix: use type * fix: undefined type issue * fix: simplify check * Update service setup for my pages infra * chore: charts update dirty files * Remove from infra * undo rename --------- Co-authored-by: albinagu <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Ásdís Erna Guðmundsdóttir <[email protected]> Co-authored-by: Þorkell Máni Þorkelsson <[email protected]> Co-authored-by: Svanhildur Einarsdóttir <[email protected]> Co-authored-by: Kristofer <[email protected]> Co-authored-by: helgifr <[email protected]> Co-authored-by: hfhelgason <[email protected]> Co-authored-by: veronikasif <[email protected]> Co-authored-by: Rafn Árnason <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Rúnar Vestmann <[email protected]> Co-authored-by: mannipje <[email protected]> Co-authored-by: unakb <[email protected]> Co-authored-by: juni-haukur <[email protected]> Co-authored-by: birkirkristmunds <[email protected]> Co-authored-by: Kristján Albert <[email protected]>
No more formatting or lint errors/warnings in shell scripts!!
Depends on #16241
Summary by CodeRabbit
These changes collectively enhance the reliability and maintainability of the CI/CD processes.