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

Improve checks on github.ref fields #424

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Conversation

ollaw
Copy link
Contributor

@ollaw ollaw commented Jun 6, 2024

This PR arises from the problem of having "pull" substring inside your branch name.

In particular, having a branch name like ISSUE-XXX/task/enable-pull-lorem-ipsum causes a crash in the action since the if inside GITHUB_PR_NUMBER property wrongly assumes that github.ref refers to a PR, and tries to cast to integer the PR number.

Goal of this PR is to make the check a bit stronger, for both GITHUB_PR_NUMBER and GITHUB_BRANCH_NAME (since it's easier to incur in same problem with a tags that contains "heads" substring inside it) checking github.ref head rather than randomly checking that a word is inside it.

Signed-off-by: Olla Gabriele <[email protected]>
Copy link

github-actions bot commented Jun 6, 2024

End-to-end public repo

Admin commands cheatsheet:

  • /e2e (in approved PR review body): Trigger end-to-end tests on external contributions
  • /invite (in comment): Invite the author & admins to the end-to-end private repo

Copy link

github-actions bot commented Jun 6, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  coverage_comment
  settings.py
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

Damn, of course 🤦

Thanks for the contribution, would you mind adding a couple of tests in https://github.com/py-cov-action/python-coverage-comment-action/blob/main/tests/unit/test_settings.py#L126-L145 to ensure we don't get a regression ? Thanks !

@ewjoachim
Copy link
Member

I think I never thought it would happen because I've mainly tested with on: pull_request whereas I guess you're experiencing this with on: push ?

@ollaw
Copy link
Contributor Author

ollaw commented Jun 6, 2024

whereas I guess you're experiencing this with on: push ?

Exactly, I also unfairly blamed GtiHub at first, I thought there was some problem on push / pull_request triggers 😄

would you mind adding a couple of tests [...] ?

Sure, take a look if that's okay.

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

Perfect 🤩

@ewjoachim ewjoachim merged commit 44f4df0 into py-cov-action:main Jun 6, 2024
3 checks passed
@ewjoachim
Copy link
Member

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.

2 participants