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

Change diff reference for admin UI check #1306

Closed
wants to merge 1 commit into from

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Dec 16, 2024

Previously, this was pointing to the file from the base commit of the PR. While this works fine for PRs that target the "main" branch, when a PR targets any other branch, we'll want to reference the branch's name rather than a specific commit SHA.

At least that's what I think is going on.

Previously, this was pointing to the file from the base commit
of the PR. While this works fine for PRs that target the "main"
branch, when a PR targets any other branch, we'll want to reference
the branch's name rather than a specific commit SHA.

At least that's what I think is going on.
@owi92 owi92 added the changelog:nope Not worth mentioning in the changelog label Dec 16, 2024
@LukasKalbertodt
Copy link
Member

Mh now I'm confused. I thought main was hardcoded and that's the problem, but the ref seems correct to me.

I looked at #1305 to figure this out and noticed that for your last force push, there is no admin UI comment anymore, so it works there. For that force push, graphql-inspector was instructed to compare 606d792c64d618460e116aa1397fc474c29e62ab and 119ad94e5bc598d53636d65eaf869f7d6ef753a4, which are the correct commits. And indeed, no diff was noticed. For your first push (the one that caused a comment to be posted), 45a70db39d13e3a45c7d11193016737ce92570c3 and 3d53a9fb3baf4eae6d216a7b096a7c149364df47 were compared. (You can see which commits were compared in the workflow log).

So I think the annoying comments we saw in the past weeks were just due to some random wrong base branches or whatever. I believe everything should be already fine the way it is now. #1295 also did not get a comment.

So we will just close this PR?

@owi92
Copy link
Member Author

owi92 commented Dec 18, 2024

Closed as per the above comment

@owi92 owi92 closed this Dec 18, 2024
@owi92 owi92 deleted the fix-admin-ui-check-target branch January 17, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:nope Not worth mentioning in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants