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.

Sign up for GitHub

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

Jump to bottom

fix(IDX): update diff logic to work outside of PRs #1745

Closed
wants to merge 15 commits into from
Closed

fix(IDX): update diff logic to work outside of PRs #1745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6d2e312
fix(IDX): update diff logic to work outside of PRs
cgundy Sep 30, 2024
b875bfc
IDX GitHub Automation
Sep 30, 2024
44aa073
remove merge_base_sha
cgundy Sep 30, 2024
9643d92
IDX GitHub Automation
Sep 30, 2024
c0fa3f8
fix
cgundy Sep 30, 2024
6057bf2
try again
cgundy Sep 30, 2024
a76302c
debug;
cgundy Sep 30, 2024
2aa68b5
rename
cgundy Sep 30, 2024
e26a05e
rename again
cgundy Sep 30, 2024
03cd834
Revert "rename again"
cgundy Sep 30, 2024
a4e5fe7
update to head
cgundy Sep 30, 2024
ded9125
udpate candid checks
cgundy Sep 30, 2024
af39b6e
keep MERGE_BASE_SHA
cgundy Sep 30, 2024
e926b8f
update build-ic
cgundy Sep 30, 2024
b85d8e9
IDX GitHub Automation
Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/actions/bazel-test-all/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ runs:
BAZEL_CI_CONFIG: ${{ inputs.BAZEL_CI_CONFIG }}
BAZEL_EXTRA_ARGS: "${{ inputs.BAZEL_EXTRA_ARGS }} ${{ inputs.BAZEL_EXTRA_ARGS_RULES }}"
BAZEL_STARTUP_ARGS: ${{ inputs.BAZEL_STARTUP_ARGS }}
CI_PULL_REQUEST_TARGET_BRANCH_NAME: ${{ github.event.pull_request.base.ref }}
TARGET_BRANCH_NAME: ${{ github.event.pull_request.base.ref }}
MERGE_BASE_SHA: ${{ github.event.pull_request.base.sha }}
DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
HONEYCOMB_API_TOKEN: ${{ inputs.HONEYCOMB_API_TOKEN }}
SSH_PRIVATE_KEY: ${{ inputs.SSH_PRIVATE_KEY }}
2 changes: 2 additions & 0 deletions .github/workflows-source/ci-main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ jobs:
BAZEL_COMMAND: "build"
RUN_ON_DIFF_ONLY: ${{ !contains(github.event.pull_request.labels.*.name, 'CI_ALL_BAZEL_TARGETS') }}
MERGE_BASE_SHA: ${{ github.event.pull_request.base.sha }}
TARGET_BRANCH_NAME: ${{ github.event.pull_request.base.ref }}
DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
- name: Upload build-ic.tar
uses: actions/upload-artifact@v4
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/ci-main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,8 @@ jobs:
BAZEL_COMMAND: "build"
RUN_ON_DIFF_ONLY: ${{ !contains(github.event.pull_request.labels.*.name, 'CI_ALL_BAZEL_TARGETS') }}
MERGE_BASE_SHA: ${{ github.event.pull_request.base.sha }}
TARGET_BRANCH_NAME: ${{ github.event.pull_request.base.ref }}
DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
- name: Upload build-ic.tar
uses: actions/upload-artifact@v4
with:
Expand Down
6 changes: 5 additions & 1 deletion ci/bazel-scripts/diff.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ set -euo pipefail
set -x
cd "$(git rev-parse --show-toplevel)"

MERGE_BASE="${MERGE_BASE_SHA:-HEAD}"
# since default branch is not the same in ic-private, we can't set it to master, throw error if missing
git fetch origin $DEFAULT_BRANCH
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would cause quite some downloads since we don't fetch all history when we checkout the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it's only pulling master... Or should I specify a couple commits like we have in the checkout step?

Copy link
Member

Choose a reason for hiding this comment

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

Using --depth 512 should be safe enough.

TARGET_BRANCH_NAME="${TARGET_BRANCH_NAME:-$DEFAULT_BRANCH}"
TARGET_BRANCH_SHA=$(git rev-parse origin/$TARGET_BRANCH_NAME)
MERGE_BASE=$(git merge-base HEAD "$TARGET_BRANCH_SHA")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this cause the problem we discussed yesterday where HEAD on PRs points to a merge between the PR branch and the target branch meaning that this git merge-base HEAD "$TARGET_BRANCH_SHA" will return that merge commit which then results in COMMIT_RANGE being HEAD.. which then causes DIFF_FILES to become empty?

I'm doing these git calculations in my head so I could be wrong here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I realise I'm wrong. If HEAD points to a merge of the PR with the target branch git merge-base HEAD "$TARGET_BRANCH_SHA" should result in $TARGET_BRANCH_SHA.
Meaning COMMIT_RANGE=$TARGET_BRANCH_SHA... So that could actually work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I double checked this and from my understanding git merge-base will always return the common ancestor (so not the merge commit). It's the same as MERGE_BASE_SHA: ${{ github.event.pull_request.base.sha }}. The one that could return a merge commit would be the latest commit, which is CI_COMMIT_SHA: ${{ github.sha }}.

COMMIT_RANGE=${COMMIT_RANGE:-$MERGE_BASE".."}
DIFF_FILES=$(git diff --name-only "${COMMIT_RANGE}")

Expand Down
2 changes: 1 addition & 1 deletion ci/bazel-scripts/main.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ for pattern in "${protected_branches[@]}"; do
done

# if we are on a protected branch or targeting a rc branch we set ic_version to the commit_sha and upload to s3
if [[ "${IS_PROTECTED_BRANCH:-}" == "true" ]] || [[ "${CI_PULL_REQUEST_TARGET_BRANCH_NAME:-}" == "rc--"* ]]; then
if [[ "${IS_PROTECTED_BRANCH:-}" == "true" ]] || [[ "${TARGET_BRANCH_NAME:-}" == "rc--"* ]]; then
ic_version_rc_only="${CI_COMMIT_SHA}"
s3_upload="True"
RUN_ON_DIFF_ONLY="false"
Expand Down
2 changes: 1 addition & 1 deletion ci/src/git_changes/git_changes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

def target_branch() -> str:
default_branch = os.getenv("CI_DEFAULT_BRANCH", "master")
return os.getenv("CI_PULL_REQUEST_TARGET_BRANCH_NAME", default_branch)
return os.getenv("TARGET_BRANCH_NAME", default_branch)


def git_fetch_target_branch(git_repo, max_attempts=10):
Expand Down
Loading