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

README build-status badge reflects status of arbitrary (most recent) CI run for any PR, rather than master branch #3610

Closed
benclifford opened this issue Sep 7, 2024 · 5 comments · Fixed by #3614

Comments

@benclifford
Copy link
Collaborator

Describe the bug
The build status badge in README.rst reflects the build status of the most recent CI run on any branch. In general, CI runs on arbitrary PRs aren't expected to pass: the reason for running them on branches is to help flush out bugs, not prove how perfect the code is before being submitted.

To Reproduce
Make a PR build that fails. Look at README.rst on the Parsl github main page.

Expected behavior
Build status is a bit irrelevant here with our aggressive policy of requiring passing builds before merges. But it shouldn't imply that "Parsl is broken".

Environment
github

@benclifford benclifford changed the title README build-status badge reflects status of arbitrary (most recent) CI run for any PR README build-status badge reflects status of arbitrary (most recent) CI run for any PR, rather than master branch Sep 9, 2024
@Harichandra-Prasath
Copy link
Contributor

It is mentioned that if there is no branch mentioned in the status badge url as the query parameter, it will take the status of CI Runs of the default branch. I am not sure whether this is the same when the requests are against non-master branches. (like when a CI fails on pr request against a non-master branch). Even after successfull run of #3613 , the status remains failed. Maybe we can try adding the branch parameter in the badge url. Further info - https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/monitoring-workflows/adding-a-workflow-status-badge#using-the-branch-parameter

@benclifford
Copy link
Collaborator Author

@Harichandra-Prasath ok, try that

@Harichandra-Prasath
Copy link
Contributor

Harichandra-Prasath commented Sep 13, 2024

Upon working on it, got to know few things.

  1. For the current CI setup, we dont have way to capture the status of Prs made against the master branch because for ?branch=master&event=pull_request, it is expecting a pr from the master branch which is very high unlikely. I believe there is no event to capture the Prs made against a branch as of now. For more info, check here; Correct workflow badge 'event' example github/docs#12331 (comment).
  2. Workaround to this is to have a CI run on push event of master branch and mention ?branch=master&event=push in the badge url since successful merge is a push in the target branch, but that will add another ci run.
  3. If we do not want to modify the workflow, we can have a separate branch say X where the arbitrary prs will be made and if found to be good , it will get merged on X and that branch X will make a pr to master branch. In this case, we have to change the badge url to ?branch=X&event=pull_request. Even this will add another ci run ; one from the arbitrary pr to X and another from X to master but X to master will not fail

@benclifford
Copy link
Collaborator Author

ok, that's an interesting observation.

Maybe the right thing to do is remove the badge entirely

We have an enforced policy that PRs can only be merged when they pass PR so I'm not really sure who this badge is aimed at/what purpose this badge serves.

@Harichandra-Prasath
Copy link
Contributor

Yes, I agree on removing the status badge as it is conveying the wrong message to users as parsl "failing"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants