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

Fix origin repo check that bypasses Docker Login on PRs originating from forks. #15724

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

kzvezdarov
Copy link
Contributor

@kzvezdarov kzvezdarov commented Oct 16, 2024

PR #15708 attempts to bypass the Docker Login step in the Python tests workflow, which is currently failing on PRs originating from forks, e.g. this one #15687 . The conditional doesn't seem to work correctly as the test failures reoccur even after the fix is applied. I'm not extremely familiar with GitHub Actions, but it seems that the github.repository context variable holds the base repository name, rather than the head repository, meaning that the check always passes and docker login is always attempted.

This PR changes the comparison to be explicitly against the head repository as recorded on the pull request event - this should correctly hold the fork name, leading to a bypass of the login step for pull requests triggered by a remote fork. The changes are mostly based on looking through threads such as https://github.com/orgs/community/discussions/25217.

Closes #15725

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

@kzvezdarov kzvezdarov changed the title Fixed origin repo check that bypasses Docker Login on PRs originating from forks. Fix origin repo check that bypasses Docker Login on PRs originating from forks. Oct 16, 2024
@github-actions github-actions bot added the bug Something isn't working label Oct 16, 2024
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link

codspeed-hq bot commented Oct 16, 2024

CodSpeed Performance Report

Merging #15724 will not alter performance

Comparing kzvezdarov:fix-docker-login-on-fork (d06ab7d) with main (123f880)

Summary

✅ 3 untouched benchmarks

@zzstoatzz zzstoatzz added the great writeup This is a wonderful example of our standards label Oct 16, 2024
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

thanks so much for the thorough write up @kzvezdarov !

@desertaxle desertaxle merged commit 739d3e0 into PrefectHQ:main Oct 16, 2024
34 checks passed
@kzvezdarov kzvezdarov deleted the fix-docker-login-on-fork branch October 16, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working great writeup This is a wonderful example of our standards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker Login step fails in PRs originating from forks.
3 participants