Skip to content

Build/release/fix execution rights#1550

Draft
spike-rabbit wants to merge 5 commits intomainfrom
build/release/fix-execution-rights
Draft

Build/release/fix execution rights#1550
spike-rabbit wants to merge 5 commits intomainfrom
build/release/fix-execution-rights

Conversation

@spike-rabbit
Copy link
Member

Describe in detail what your merge request does and why. Add relevant
screenshots and reference related issues via Closes #XY or Related to #XY.


Running `chmod u+x` on the shell files.
Interpolating GitHub Actions variables in shell script does not work.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request fixes an issue in the release script where the check for the default branch was not working correctly. By passing the information as a script argument, the logic is now correct and more robust. I have one suggestion to further improve the robustness of this check by making it case-insensitive.

DEPLOY_LATEST="false"

if [[ "${{ github.ref_name }}" == "${{ github.event.repository.default_branch }}" ]]; then
if [[ "$IS_DEFAULT_BRANCH" == "true" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The string comparison to check if this is the default branch is case-sensitive. To make the script more robust against potential variations in the input (e.g., True or TRUE), consider converting the variable to lowercase before the comparison. This will prevent unexpected behavior if the value passed from the CI workflow changes. Note that the ${VAR,,} syntax for case conversion requires bash version 4 or newer, which is available on GitHub-hosted runners.

Suggested change
if [[ "$IS_DEFAULT_BRANCH" == "true" ]]; then
if [[ "${IS_DEFAULT_BRANCH,,}" == "true" ]]; then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments