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

chore(IDX): use github.ref_protected where applicable #3936

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nmattia
Copy link
Contributor

@nmattia nmattia commented Feb 13, 2025

This means we don't have to keep track of the protected branches and means we can use unified logic for protected ref detection.

This means we don't have to keep track of the protected branches and
means we can use unified logic for protected ref detection.
@nmattia nmattia requested a review from cgundy February 13, 2025 11:40
@nmattia nmattia requested a review from a team as a code owner February 13, 2025 11:40
@github-actions github-actions bot added the chore label Feb 13, 2025
@github-actions github-actions bot added the @idx label Feb 13, 2025
Comment on lines -14 to -23
protected_branches=("master" "rc--*" "hotfix-*" "master-private")

# if we are on a protected branch or targeting a rc branch we set ic_version to the commit_sha and upload to s3
for pattern in "${protected_branches[@]}"; do
if [[ "$BRANCH_NAME" == $pattern ]]; then
IS_PROTECTED_BRANCH="true"
break
fi
done

Copy link
Collaborator

@basvandijk basvandijk Feb 13, 2025

Choose a reason for hiding this comment

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

Unfortunately and confusingly setting IS_PROTECTED_BRANCH=${{ github.ref_protected }} as an alternative to the above is not enough.

In ic-private the hotifx-* branch is not protected. This means that if folks follow the "How to handle fixes for IC security vulnerabilities?" procedure, which specifies to push the hotfix to a hotfix-* branch and then expects the artifacts to be uploaded (s3_upload="True"), their artifacts won't be uploaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@basvandijk arguably this is an issue in ic-private where protected_branches is then not aptly named, and/or an issue with the hotfix procedure. I'll see what I can do about that.

Copy link
Collaborator

@basvandijk basvandijk Feb 13, 2025

Choose a reason for hiding this comment

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

Note that we shouldn't protect hotfix-* in ic-private since that would prevent folks from creating hotfixes.

So I think it's indeed a naming issue. Maybe we should use a different wordt than "PROTECTED" above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe:

  • protected_branches -> release_branches
  • IS_PROTECTED_BRANCH -> IS_RELEASE_BRANCH

@nmattia nmattia marked this pull request as draft February 13, 2025 11:59
@@ -73,3 +73,4 @@ runs:
MERGE_BASE_SHA: ${{ github.event.pull_request.base.sha }}
SSH_PRIVATE_KEY_BACKUP_POD: ${{ inputs.SSH_PRIVATE_KEY_BACKUP_POD }}
GPG_PASSPHRASE: ${{ inputs.GPG_PASSPHRASE }}
IS_PROTECTED_BRANCH: ${{ github.ref_protected }}
Copy link
Member

Choose a reason for hiding this comment

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

@nmattia it looks like we had this logic before, but it didn't work as expected:

#1504

I think we would need to test this first to make sure this variable does show up as true on protected branches.

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.

3 participants