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

replace master references with default branch name fetched from github api #141

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

Conversation

sophlanz
Copy link

@sophlanz sophlanz commented Oct 17, 2024

Jira
https://clever.atlassian.net/browse/DD-5739

Overview
Testing changes to no longer reference master in favor of referencing the default branch returned from the github API.

Testing

Default branch correctly outputs master for the ci-scripts repo
Screenshot 2024-10-17 at 1 49 58 PM

Rollout

@sophlanz sophlanz marked this pull request as ready for review November 11, 2024 18:20
@sophlanz sophlanz requested a review from a team as a code owner November 11, 2024 18:20
@sophlanz sophlanz requested review from jakegut and removed request for a team November 11, 2024 18:20
@pickabot pickabot bot requested a review from andruwm November 11, 2024 18:21
@andruwm andruwm requested review from a team and sriram-clever and removed request for a team November 11, 2024 18:39
@andruwm
Copy link
Contributor

andruwm commented Nov 11, 2024

Tagging security since this also adds new credentials to the CI pipeline.

Copy link
Contributor

@andruwm andruwm left a comment

Choose a reason for hiding this comment

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

A few comments

if [[ $BRANCH != "master" ]]; then
: ${CIRCLE_PROJECT_REPONAME?"Missing required env var"}
REPO=$CIRCLE_PROJECT_REPONAME
: ${GITHUB_TOKEN?"Missing required env var"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this token need to be added and rotated in all CI projects?

DEFAULT_BRANCH=$(curl -s -H "Authorization: token $GITHUB_TOKEN" \
"https://api.github.com/repos/Clever/$REPO" | jq -r '.default_branch')

if [[ $BRANCH != $DEFAULT_BRANCH ]]; then
echo "Skipping sync for non-master branch"
Copy link
Contributor

Choose a reason for hiding this comment

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

The text here should also get updated, so that if the primary branch is ever named main this error message wouldn't be confusing.

@@ -14,7 +16,7 @@ echo "2) Run the following command, replacing CIRCLE_API_TOKEN with the token fr
echo "curl -u CIRCLE_API_TOKEN: \\
-X POST \\
--header \"Content-Type: application/json\" -d '{
\"branch\": \"master\",
\"branch\": \"${DEFAULT_BRANCH}\",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to accompany this with catapult changes? This would probably not behave as expected with just the CI changes if a team were to change their primary branch name.

@pickabot pickabot bot requested a review from tinydeltas November 12, 2024 05:21
@sriram-clever sriram-clever removed their request for review November 12, 2024 05:22
@jakegut jakegut removed their request for review November 26, 2024 17:00
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.

3 participants