Skip to content

Fix: Fixes hardcoded commit SHA on test_install_vcs_git.py #13492

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

Merged
merged 4 commits into from
Jul 20, 2025

Conversation

sepehr-rs
Copy link
Contributor

Fixes #13491,

I tried to fix this bug to the best of my ability. I’d appreciate any feedback or suggestions on how to make it better.

@notatallshaw
Copy link
Member

@ichard26 this appears to be your commit: https://github.com/pypa/pip-test-package/commits/master/

@notatallshaw notatallshaw added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jul 19, 2025
@notatallshaw
Copy link
Member

@sepehr-rs I don't think we need a news item here, or at least just make it a "trivial" news item.

@sepehr-rs
Copy link
Contributor Author

@sepehr-rs I don't think we need a news item here, or at least just make it a "trivial" news item.

Got it, I’ve gone ahead and removed it.

@sepehr-rs
Copy link
Contributor Author

@ichard26 this appears to be your commit: https://github.com/pypa/pip-test-package/commits/master/

I think this PR serves as a temporary fix. A more sustainable approach would be to avoid hardcoding, since every time there's a new commit on that project, a new PR will be needed here to address it.

@notatallshaw
Copy link
Member

Thanks for the PR @sepehr-rs, probably you are correct, I'm going to hold of from merging for just a little bit to see if @ichard26 has any comment on something we could do better here.

@ichard26
Copy link
Member

ichard26 commented Jul 20, 2025

Could we clone a specific tag or branch on the pip-test-package repository that's meant to be frozen for the SHA test? We can call it frozen or even do-not-update (and I can set up protection rules to block inadvertent pushes). While I don't imagine we'd update the repository often, it would be nice to be able to make updates w/o having to update this test every time.

If that sounds reasonable, I'm happy to set this up on pip-test-package.

@notatallshaw
Copy link
Member

The test seems to be specifically for "Clone the default branch", there's already tests for cloning a tag and specific commits.

To be more flexible I think the way this test works would need to change, rather than checking if some specific commit was sourced rather run some git command that checks the default branch has been sourced.

@sepehr-rs
Copy link
Contributor Author

Hi @notatallshaw and @ichard26, thanks for the great discussion here!
I see that @ichard26’s suggestion to use a separate frozen branch could help avoid frequent updates, while @notatallshaw’s point about making the test more flexible sounds like a long-term improvement.
Would it make sense to merge the temporary fix for now to unblock things, and then open a dedicated issue or discussion to explore the frozen branch and more flexible test options in depth?
I’m happy to assist with either step if that would be helpful. Please let me know if this sounds like a reasonable way forward!

@notatallshaw notatallshaw merged commit 7f76f2b into pypa:main Jul 20, 2025
29 checks passed
@notatallshaw
Copy link
Member

Yeah, let's fix CI now and have a follow up PR later.

@sepehr-rs
Copy link
Contributor Author

Yeah, let's fix CI now and have a follow up PR later.

That sounds great, should I open up an issue about it?

@notatallshaw
Copy link
Member

You're welcome to if you would like, but there is no obligation.

@sepehr-rs
Copy link
Contributor Author

You're welcome to if you would like, but there is no obligation.

Sure, I just made one :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All install_git_vcs tests are failing because of hardcoded commit SHA
3 participants