chore: prevent revision check from retrying unnecessary #1315
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/tools/revisionVerifier.sh (3)
41-45: LGTM: Improved error handling and consistent use of exit codes.The addition of a specific check for the "Failed" state enhances error handling, and the use of exit code constants improves consistency and readability.
Consider adding a comment explaining the conditions for
EXIT_NOT_READYto improve clarity:if [[ $health_state == "Healthy" && ($running_state == "Running" || $running_state == "RunningAtMaxScale") ]]; then return $EXIT_SUCCESS else + # Return EXIT_NOT_READY if the revision is not healthy or not in a running state return $EXIT_NOT_READY fiAlso applies to: 49-51
57-76: LGTM: Improved control flow with case statement.The refactoring of the main loop to use a case statement significantly improves the readability and maintainability of the script. The logic for handling different states is clear and well-structured.
To address the static analysis warnings and improve robustness, consider quoting the variables in the case patterns:
case $result in - $EXIT_SUCCESS) + "$EXIT_SUCCESS") echo "Revision $revision_name is healthy and running" exit $EXIT_SUCCESS ;; - $EXIT_FAILED) + "$EXIT_FAILED") echo "Revision $revision_name has failed. Exiting." exit $EXIT_FAILED ;; - $EXIT_NOT_READY) + "$EXIT_NOT_READY") echo "Attempt $attempt: Waiting for revision $revision_name ..." sleep 10 # Sleep for 10 seconds attempt=$((attempt+1)) ;; esacThis change will prevent potential issues if the variables contain special characters or spaces.
🧰 Tools
🪛 Shellcheck
[warning] 63-63: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
[warning] 67-67: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
[warning] 71-71: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
Line range hint
21-76: Overall improvements enhance script robustness and maintainability.The changes made to this script align well with the PR objectives of preventing unnecessary retries for failed revisions. The introduction of clear exit codes, improved error handling, and the refactored main loop using a case statement all contribute to a more robust and maintainable script.
These modifications should effectively address the issue of continuous retrying after a revision has failed, improving the efficiency of the revision checking process.
To further improve the script's robustness, consider adding logging functionality. This could help with debugging and monitoring the script's behavior in production. For example:
log_message() { echo "[$(date +'%Y-%m-%d %H:%M:%S')] $1" >> /path/to/logfile.log } # Then use it in the script: log_message "Attempt $attempt: Waiting for revision $revision_name ..."This would provide valuable insights into the script's execution over time.
🧰 Tools
🪛 Shellcheck
[warning] 63-63: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
[warning] 67-67: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
[warning] 71-71: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/tools/revisionVerifier.sh (2 hunks)
🧰 Additional context used
🪛 Shellcheck
.github/tools/revisionVerifier.sh
[warning] 63-63: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
[warning] 67-67: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
[warning] 71-71: Quote expansions in case patterns to match literally rather than as a glob.
(SC2254)
🔇 Additional comments (1)
.github/tools/revisionVerifier.sh (1)
21-24: LGTM: Well-defined exit codes improve script clarity.The introduction of these exit code constants enhances the script's readability and maintainability. The use of
readonlyis a good practice to prevent accidental modifications.
Description
The revision check kept on retrying even though the revision had failed
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Bug Fixes
Refactor