chore(scripts): Formatting #16241
chore(scripts): Formatting #16241
Conversation
WalkthroughThis pull request includes extensive formatting changes across multiple shell scripts to improve readability and maintainability. Modifications primarily involve adjusting whitespace, indentation, and alignment of commands, while maintaining the original functionality of the scripts. Notably, some scripts have enhanced control flow structures, and a few have minor logic adjustments related to variable assignments. Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
infra/scripts/ci/diff-chart-values-all-charts.sh (1)
8-11: LGTM: Improved loop formatting with a suggestionThe reformatting of the loop to a multi-line structure enhances readability while maintaining the same functionality. This change aligns well with the PR's formatting objectives.
For future improvements, consider replacing the Python one-liner with a native bash solution for listing directories. This could potentially improve performance and reduce dependencies. For example:
for chart in $( cd "$ROOT/infra/src/uber-charts/" && find . -maxdepth 1 -type f -name '*.json' -exec basename {} .json \; ); do # ... rest of the loop doneThis suggestion is outside the scope of the current PR and can be considered for future refactoring.
scripts/ci/generate-chunks.sh (1)
9-14: LGTM with a suggestion: Consider making the skip condition more flexibleThe reformatting of the if statement improves readability. The use of environment variables with default values enhances script flexibility.
However, the skip condition
"$SKIP_TESTS_ON_BRANCH" == "7913-$BRANCH"seems very specific. Consider making this more flexible, perhaps using a pattern match or a configurable value.Example:
if [[ "$BRANCH" =~ $SKIP_TESTS_PATTERN ]]; thenThis would allow for more dynamic control over which branches skip tests.
scripts/ci/30_test.sh (1)
Line range hint
1-56: Consider optimizing script efficiencyWhile the formatting changes improve readability, consider the following suggestions to enhance script efficiency:
- Use
[[ ]]instead of[ ]for condition testing, as it's more efficient and less error-prone.- Consider using a case statement instead of multiple if-else conditions for setting
IS_FLAKY_TESTandEXTRA_OPTS.- Use
mapfileorreadarrayto read the arrays instead of declaring them inline, which can be more efficient for larger arrays.These changes could further improve the script's efficiency while maintaining its readability and maintainability.
scripts/ci/00_prepare-base-tags.sh (1)
Line range hint
1-38: Overall: Well-structured and maintainable script with room for minor improvementThe script demonstrates good practices in terms of efficiency, readability, and maintainability. It makes excellent use of environmental variables and integrates well with GitHub Actions. The error handling and reporting mechanisms enhance its robustness in a CI/CD context.
While the script indirectly supports NX workspace integration through the variables it sets, consider adding a comment explaining how these variables are used in the NX context. This would further improve the script's maintainability and alignment with the guideline for NX workspace integration.
scripts/create-secret.sh (7)
43-47: Improved validate_empty function formatting and logicThe changes to the
validate_emptyfunction enhance readability and robustness:
- Removing the space before the parentheses in the function declaration improves consistency.
- Using
[[ ]]instead of[ ]is a good practice for more predictable behavior.- Moving the
thenkeyword to the same line as the condition improves readability.These changes align well with the script efficiency and readability guidelines.
Consider using
[[ -z $1 ]]instead of[[ -d $1 ]]to check for an empty parameter, as it's more idiomatic for string checking in shell scripts.
55-64: Improved validate_whitespace function formattingThe changes to the
validate_whitespacefunction enhance readability and consistency:
- Removing the space before the parentheses in the function declaration improves consistency.
- Reformatting the conditional statements improves readability.
These changes align well with the script efficiency and readability guidelines.
Consider using the
__error_exitfunction for error handling instead of directly callingexit 0. This would provide more consistent error handling throughout the script. For example:if [[ $1 = "$ILLEGAL_CHARS" ]]; then __error_exit "Whitespaces are not allowed" fi
66-76: Improved validate_chars function formattingThe changes to the
validate_charsfunction enhance readability and consistency:
- Removing the space before the parentheses in the function declaration improves consistency.
- Reformatting the conditional statements improves readability.
These changes align well with the script efficiency and readability guidelines.
Similar to the previous suggestion, consider using the
__error_exitfunction for error handling instead of directly callingexit 0. This would provide more consistent error handling throughout the script. For example:if [[ ! $1 =~ $ALPHANUMERIC_DASH$ONE_OR_MORE ]]; then __error_exit "Secret name can only contain letters, numbers, hyphens and underscores" fi
Line range hint
78-91: Improved validate_length function formattingThe changes to the
validate_lengthfunction enhance readability and consistency:
- Removing the space before the parentheses in the function declaration improves consistency.
- Reformatting the conditional statements improves readability.
These changes align well with the script efficiency and readability guidelines.
Consider the following improvements:
- Use the
__error_exitfunction for consistent error handling:if ((${#1} < MIN_LENGTH || ${#1} > MAX_LENGTH)); then __error_exit "Too short, should be 6-256 characters long." fi- Use the
$MAX_LENGTHvariable in the error message instead of hardcoding "256":__error_exit "Too short, should be $MIN_LENGTH-$MAX_LENGTH characters long."This change would make the error message more maintainable if the
MAX_LENGTHvalue changes in the future.
Line range hint
94-128: Improved prepare_secret function formattingThe changes to the
prepare_secretfunction enhance readability and consistency:
- Removing the space before the parentheses in the function declaration improves consistency.
- Reformatting the conditional statements improves readability.
These changes align well with the script efficiency and readability guidelines.
Consider the following improvements for better user input handling:
- Use
read -rinstead ofread -erfor better portability across different shells.- Add input validation for the SecureString and Add tags prompts. For example:
while true; do read -rp "${BLUE}SecureString [Y/n]? ${RESET}" yn case $yn in [Yy]*|"" ) SECRET_TYPE="SecureString"; break;; [Nn]* ) SECRET_TYPE="String"; break;; * ) echo "Please answer yes or no.";; esac doneThis change would make the script more robust against unexpected user inputs.
130-142: Improved create_secret function formattingThe changes to the
create_secretfunction enhance readability and consistency:
- Removing the space before the parentheses in the function declaration improves consistency.
- Reformatting the conditional statement improves readability.
These changes align well with the script efficiency and readability guidelines.
Consider the following improvements for better security and error handling:
- Use double quotes around variables to prevent word splitting and globbing:
CMD="aws ssm put-parameter --name \"$SSM_PREFIX$SECRET_NAME\" --value \"$SECRET_VALUE\" --type \"$SECRET_TYPE\""- Add error handling for the AWS CLI command:
if ! eval "$CMD"; then __error_exit "Failed to create secret" fiThese changes would make the script more secure and robust against potential errors.
Line range hint
1-168: Overall improvement in script formatting and readabilityThe changes made to this script have significantly enhanced its readability and maintainability:
- Consistent function declaration style throughout the script.
- Improved formatting of conditional statements and case blocks.
- Better overall code structure and indentation.
These changes align well with the provided coding guidelines, particularly in terms of script efficiency, readability, and maintainability. The script continues to use environmental variables and follows a modular structure, which is good for flexibility and maintainability.
While the functional aspects of the script remain unchanged, the improved formatting will make it easier for developers to understand and modify the code in the future.
To further improve the script, consider:
- Implementing more consistent error handling using the
__error_exitfunction throughout.- Enhancing input validation for user prompts to make the script more robust.
- Using double quotes around variables consistently to prevent word splitting and globbing issues.
- Adding comments to explain complex logic or important decisions in the script.
These suggestions would further enhance the script's robustness and maintainability, aligning even more closely with best practices for shell scripting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (36)
- apps/financial-aid/api/scripts/run-xroad-proxy.sh (1 hunks)
- apps/services/search-indexer/bin/generate-es-config.sh (1 hunks)
- infra/scripts/build-docker-container.sh (2 hunks)
- infra/scripts/ci/diff-chart-values-all-charts.sh (1 hunks)
- infra/scripts/ci/diff-chart-values.sh (1 hunks)
- infra/scripts/ci/git-check-dirty.sh (0 hunks)
- infra/scripts/ci/test-unit.sh (1 hunks)
- infra/scripts/container-scripts/destroy-dbs.sh (1 hunks)
- infra/scripts/format-yaml.sh (1 hunks)
- infra/scripts/helm-diff.sh (1 hunks)
- scripts/ci/00_prepare-base-tags.sh (3 hunks)
- scripts/ci/10_prepare-docker-deps.sh (1 hunks)
- scripts/ci/10_prepare-host-deps.sh (1 hunks)
- scripts/ci/20_check-formatting.sh (1 hunks)
- scripts/ci/30_test.sh (1 hunks)
- scripts/ci/50_upload-coverage.sh (2 hunks)
- scripts/ci/90_docker-express.sh (1 hunks)
- scripts/ci/90_docker-jest.sh (1 hunks)
- scripts/ci/90_docker-next.sh (1 hunks)
- scripts/ci/90_docker-playwright.sh (1 hunks)
- scripts/ci/90_docker-static.sh (1 hunks)
- scripts/ci/_common.sh (2 hunks)
- scripts/ci/_nx-affected-targets.sh (1 hunks)
- scripts/ci/cache/generate-files.sh (1 hunks)
- scripts/ci/ci.sh (1 hunks)
- scripts/ci/docker-login-ecr.sh (1 hunks)
- scripts/ci/generate-build-chunks.sh (1 hunks)
- scripts/ci/generate-chunks.sh (1 hunks)
- scripts/ci/list-unaffected.sh (2 hunks)
- scripts/ci/retag-unaffected.sh (1 hunks)
- scripts/ci/run-affected-in-parallel.sh (1 hunks)
- scripts/ci/run-in-parallel-native.sh (1 hunks)
- scripts/ci/run-in-parallel.sh (2 hunks)
- scripts/create-secret.sh (4 hunks)
- scripts/dockerfile-assets/bash/extract-environment.sh (1 hunks)
- scripts/stop-test-proxies.sh (1 hunks)
💤 Files with no reviewable changes (1)
- infra/scripts/ci/git-check-dirty.sh
✅ Files skipped from review due to trivial changes (28)
- apps/financial-aid/api/scripts/run-xroad-proxy.sh
- apps/services/search-indexer/bin/generate-es-config.sh
- infra/scripts/build-docker-container.sh
- infra/scripts/ci/diff-chart-values.sh
- infra/scripts/ci/test-unit.sh
- infra/scripts/format-yaml.sh
- infra/scripts/helm-diff.sh
- scripts/ci/10_prepare-docker-deps.sh
- scripts/ci/10_prepare-host-deps.sh
- scripts/ci/20_check-formatting.sh
- scripts/ci/50_upload-coverage.sh
- scripts/ci/90_docker-express.sh
- scripts/ci/90_docker-jest.sh
- scripts/ci/90_docker-next.sh
- scripts/ci/90_docker-playwright.sh
- scripts/ci/90_docker-static.sh
- scripts/ci/_common.sh
- scripts/ci/cache/generate-files.sh
- scripts/ci/ci.sh
- scripts/ci/docker-login-ecr.sh
- scripts/ci/generate-build-chunks.sh
- scripts/ci/list-unaffected.sh
- scripts/ci/retag-unaffected.sh
- scripts/ci/run-affected-in-parallel.sh
- scripts/ci/run-in-parallel-native.sh
- scripts/ci/run-in-parallel.sh
- scripts/dockerfile-assets/bash/extract-environment.sh
- scripts/stop-test-proxies.sh
🧰 Additional context used
📓 Path-based instructions (5)
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/30_test.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/_nx-affected-targets.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-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/create-secret.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 (19)
infra/scripts/ci/diff-chart-values-all-charts.sh (3)
5-5: LGTM: Improved variable assignment formattingThe reformatting of the
DIRvariable assignment to a single line improves readability and aligns with common bash scripting practices. This change is consistent with the PR's objective of formatting shell scripts.
12-13: LGTM: Consistent formatting appliedThe slight reformatting of the
diff-chart-values.shexecution line maintains consistency with the overall formatting style of the script. This change adheres to the PR's objective of improving shell script formatting.
Line range hint
1-13: LGTM: Successful formatting aligned with PR objectivesThe changes in this file are entirely formatting-related and align perfectly with the PR's objective of formatting all shell scripts. The use of
shfmt, as mentioned in the PR description, is evident in the consistent and improved formatting throughout the script. These changes enhance readability and maintainability without altering the script's functionality.scripts/ci/generate-chunks.sh (3)
1-8: LGTM: Good script setup and initializationThe script follows good practices for bash scripting:
- Uses
set -euxo pipefailfor enhanced error handling and debugging.- Sets the DIR variable robustly.
- Sources a common script, promoting code reuse.
These practices contribute to script efficiency and maintainability.
Line range hint
15-21: LGTM: Efficient integration with NX and good output handlingThis section of the script demonstrates several strengths:
- Good integration with NX workspace utilities using
_nx-affected-targets.sh.- Separation of concerns by using a dedicated Node.js script for chunking.
- Efficient use of stdout for the main output and stderr for logging.
These practices contribute to the script's efficiency and maintainability, aligning well with the coding guidelines.
Line range hint
1-21: Overall assessment: Well-structured and efficient script with minor improvement opportunityThis script demonstrates good practices in bash scripting and aligns well with the provided coding guidelines:
- Efficient and readable code structure.
- Use of environmental variables for flexibility.
- Good integration with NX workspace utilities.
- Improved formatting enhances readability.
The only suggestion for improvement is to consider making the skip condition more flexible to accommodate future changes more easily.
Great job on maintaining and improving this script!
infra/scripts/container-scripts/destroy-dbs.sh (3)
11-14: Formatting improvement: Proper indentation of while loopThe changes in this segment improve the readability of the code by correctly indenting the while loop and aligning the 'done' statement with the 'while' statement. This adheres to common shell scripting style guidelines and is consistent with the PR objectives.
16-19: Formatting improvement: Consistent indentation across loopsThe changes in this segment mirror those in the previous loop, demonstrating a consistent approach to formatting throughout the script. The improved indentation and alignment enhance readability while maintaining the original functionality.
Line range hint
1-21: Verify use of 'shfmt' and consider CI integrationThe formatting changes are consistent with the PR objectives and appear to have been applied uniformly across the script. To ensure ongoing consistency:
- Can you confirm that the
shfmtcommand mentioned in the PR description was used for these changes?- Consider integrating
shfmtinto your CI process to maintain consistent formatting across all shell scripts in the repository.To verify the use of 'shfmt', you can run the following command and compare the output with the current file:
This will show how
shfmtwould format the file, allowing you to confirm if the current changes match the tool's output.scripts/ci/_nx-affected-targets.sh (2)
Line range hint
1-24: LGTM: Well-structured script with good practicesThe script is well-organized and follows good practices:
- Uses strict mode (
set -euxo pipefail) for better error handling.- Sources a common script, promoting code reuse.
- Utilizes environment variables for flexibility.
- Integrates with NX workspace utilities.
These practices align well with the coding guidelines for script efficiency, readability, and maintainability.
17-17: Approved: Syntax correction improves code qualityThe addition of a space before the closing square brackets in the if statement is a correct syntax improvement. This change:
- Ensures proper evaluation of the conditional expression.
- Improves code readability.
- Adheres to shell scripting best practices.
While the logic remains unchanged, this correction is important for maintaining high-quality, error-free scripts.
scripts/ci/30_test.sh (2)
50-55: Improved formatting enhances readabilityThe reformatting of the for loop improves the script's readability without changing its functionality. This change aligns well with our guidelines for maintaining readable and maintainable scripts.
56-56: Improved exit statement formattingThe addition of a newline before the exit statement enhances the script's readability and log clarity. This small change adheres to good shell scripting practices and improves maintainability.
scripts/ci/00_prepare-base-tags.sh (4)
4-4: LGTM: Proper script directory resolutionThe script correctly uses a reliable method to determine its own directory, which is crucial for relative path operations. This approach enhances script portability and maintainability.
13-13: LGTM: Consistent formatting and proper use of environment variablesThe formatting change improves consistency. More importantly, the script correctly uses GitHub Actions environment variables, adhering to the guideline for flexibility and integration with CI/CD systems.
26-30: LGTM: Improved formatting and robust error handlingThe formatting changes enhance readability. The error handling mechanism, including Slack notification, is a good practice for maintaining observability in the CI/CD pipeline. This aligns well with the guidelines for script maintainability and efficiency.
32-32: LGTM: Improved control flowMoving the BASE assignment inside the else block is a good improvement. It ensures that BASE is only set when BUILD_REF is valid, enhancing the script's logic and error handling. This change aligns well with the guidelines for script efficiency and maintainability.
scripts/create-secret.sh (2)
32-35: Improved error handling function formattingThe changes to the
__error_exitfunction enhance readability and maintainability:
- Removing the space before the parentheses in the function declaration adheres to common shell scripting conventions.
- Separating the
printfandexitcommands improves clarity and allows for easier debugging if needed.These changes align well with the script efficiency and readability guidelines.
144-164: Improved case statement formattingThe changes to the case statement significantly enhance readability:
- Each case is now on a separate line with proper indentation.
- The overall structure is more consistent and easier to read.
These changes align well with the script efficiency and readability guidelines, making the code more maintainable.
AndesKrrrrrrrrrrr
added
deprecated:automerge
Datadog ReportAll test runs ✅ 34 Total Test Services: 0 Failed, 33 Passed Test ServicesThis report shows up to 10 services
|
|
Affected services are: air-discount-scheme-api,air-discount-scheme-backend,api,application-system-api,download-service,financial-aid-api,financial-aid-backend,financial-aid-open-api,github-actions-cache,icelandic-names-registry-backend,judicial-system-api,judicial-system-digital-mailbox-api,judicial-system-backend,judicial-system-message-handler,judicial-system-robot-api,judicial-system-scheduler,judicial-system-xrd-api,license-api,reference-backend,regulations-admin-backend,services-auth-admin-api,services-auth-ids-api,services-auth-delegation-api,services-auth-personal-representative,services-auth-personal-representative-public,services-auth-public-api,services-contentful-entry-tagger,services-documents,services-endorsements-api,services-form-system,services-search-indexer,services-sessions,services-university-gateway,services-user-notification,services-user-profile,services-xroad-collector,skilavottord-ws,air-discount-scheme-web,auth-admin-web,consultation-portal,contentful-apps,financial-aid-web-osk,financial-aid-web-veita,judicial-system-web,skilavottord-web,web,application-system-form,island-ui-storybook,portals-admin,service-portal,system-e2e,external-contracts-tests, Deployed services: application-system-api,application-system-form,service-portal,service-portal-api,portals-admin,consultation-portal,api,web,xroad-collector,skilavottord-web,skilavottord-ws,services-documents,island-ui-storybook,download-service,icelandic-names-registry-backend,endorsement-system-api,air-discount-scheme-web,air-discount-scheme-backend,air-discount-scheme-api,regulations-admin-backend,external-contracts-tests,application-system-api-worker,license-api,services-sessions,services-sessions-worker,services-sessions-cleanup,services-university-gateway,services-university-gateway-worker. |
Format all shell scripts.
Command to reproduce:
shfmt --write $(git ls-files '**/*.sh')No refactoring in this PR, only formatting.
Summary by CodeRabbit
create-secret.sh.