-
Notifications
You must be signed in to change notification settings - Fork 43
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
Rename frontpage badges #802
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment
README.md
Outdated
@@ -2,8 +2,8 @@ | |||
|
|||
[![PyPI](https://img.shields.io/pypi/v/trieste.svg)](https://pypi.org/project/trieste) | |||
[![License](https://img.shields.io/badge/license-Apache-green.svg)](LICENSE) | |||
[![Quality checks](https://github.com/secondmind-labs/trieste/actions/workflows/develop-checks.yaml/badge.svg)](https://github.com/secondmind-labs/trieste/actions?query=workflows%3Adevelop-checks) | |||
[![Docs](https://github.com/secondmind-labs/trieste/actions/workflows/deploy.yaml/badge.svg)](https://github.com/secondmind-labs/trieste/actions/workflows/deploy.yaml) | |||
[![Release](https://img.shields.io/github/actions/workflow/status/secondmind-labs/trieste/deploy.yaml?logo=github&label=Release%20build)](https://github.com/secondmind-labs/trieste/actions/workflows/deploy.yaml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking now at our GH workflows, naming is confusing I think :/
release one actually only does docs building, it doesn't run all the tests, so its a bit misleading to call it release, quality checks below runs only the slowtests and fulldocs, not everything...
users dont care about these partial steps, if I'm considering a library I want to see that its release is healthy, all tests passing etc - thats what this release badge should be showing, not just the docs - same for develop, if I want to consider using develop branch, it should be based on all tests etc (and this one may be occasionally broken and show red, thats ok for develop I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is that there isn't a single action that includes all of the tests run on a release. There are in fact two and a half:
- "Develop checks" runs the slow tests on the release tag (as well as on every merge to develop).
- "Quality checks" doesn't explicitly get run on the release tag but since we always release from develop it's always run on the last PR that was merged before the release.
- "Deploy" deploys the documentation just on releases.
I could perhaps change the badge to point to the status of the last "develop checks" run on the current release, and explicitly link to that run? i.e. use https://img.shields.io/github/actions/workflow/status/secondmind-labs/trieste/develop-checks.yaml?logo=github&label=develop%20checks&branch=v2.0.0 which gives
Still a bit misleading (as that doesn't cover all the tests we run on the release) plus there's no automatic way to keep the explicit run link in sync with the status link.
Opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that's a problem :)
I think we should include a separate workflow with everything run on a release - this is rarely done so its fine to repeat a bunch of checks I think - and then we should put here a release badge associated with the results of that workflow
the second badge should be for develop, and perhaps for that one its ok to use develop checks, although we could do all the checks on that one as well (once PR is merged on develop branch)
another useful badge is docs building (since that often can be broken irrespective of tests), and those can point to the same workflow if fulldocs are included there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem with including everything in one workflow is that it makes it more difficult to manually run (or rerun) specific parts. Deploying the documentation is logically very different from running the release tests. That said, I could change develop checks to also run all the normal quality checks, so it's good proof that all our tests are running correctly.
Until I get round to that is it ok to merge this PR in, as I think it's still an improvement on what is currently there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we can keep the current ones and use them on develop, we can just add a more comprehensive workflow that would be run on release, no?
re docs, there I'm indifferent, we can keep those as different workflows, it is logically different thing
I think it makes more sense to do it within this PR, renaming is ultimately related to underlying workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points! I've added a release-checks workflow that will run on all releases instead of develop-checks. I haven't updated the badge to point to it yet, as I think I need to merge the new workflow to develop first before I can manually run it on the most recent release. If you're happy, tick this and I'll do that and then make a separate PR to update the badge to point to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, looks good now
Related issue(s)/PRs:
Summary
Relable and reorder the release deploy and develop check frontpage badges, so it's less scary when the develop checks happen to be failing.
Fully backwards compatible: yes
PR checklist