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

Use pull_request.head.sha for GitHub dependency graph submissions where appropriate #883

Closed
wants to merge 1 commit into from

Conversation

juxtin
Copy link

@juxtin juxtin commented Sep 11, 2023

This is my humble attempt at resolving #882.

I essentially copied what we did in github/dependency-submission-toolkit and applied it to the convention used by gradle/github-dependency-graph-gradle-plugin.

Note: I haven't yet directly verified that this works. What should happen is that dependencies are submitted on push exactly the same as they always have, and dependencies submitted for PRs should have the same SHA as the last commit on the PR branch.

closes #882

Signed-off-by: Justin Holguin <juxtin@github.com>
@bigdaz
Copy link
Member

bigdaz commented Sep 12, 2023

Thanks very much for the PR. I'm a bit unsure about changing the value of the default GITHUB_SHA environment variable, since this could have unexpected impacts in the case that someone was relying on the default value.

Instead, I think we should define a custom environment variable for this purpose, and change the dependency-graph-gradle-plugin to consume this. I can take it from here.

@juxtin
Copy link
Author

juxtin commented Sep 12, 2023

Thanks @bigdaz, that sounds like a good approach! I'll go ahead and close this.

@juxtin juxtin closed this Sep 12, 2023
@bigdaz bigdaz modified the milestone: 2.8.1 Sep 21, 2023
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.

Dependency snapshot should use pull_request.head.sha when available
2 participants