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

Rename frontpage badges #802

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Rename frontpage badges #802

merged 6 commits into from
Jan 17, 2024

Conversation

uri-granta
Copy link
Collaborator

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

  • The quality checks are all passing
  • The bug case / new feature is covered by tests
  • Any new features are well-documented (in docstrings or notebooks)

Copy link
Collaborator

@hstojic hstojic left a 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 Show resolved Hide resolved
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)
Copy link
Collaborator

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)

Copy link
Collaborator Author

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:

  1. "Develop checks" runs the slow tests on the release tag (as well as on every merge to develop).
  2. "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.
  3. "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 Release checks

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?

Copy link
Collaborator

@hstojic hstojic Jan 12, 2024

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...

Copy link
Collaborator Author

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?

Copy link
Collaborator

@hstojic hstojic Jan 15, 2024

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

Copy link
Collaborator Author

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.

@hstojic hstojic self-requested a review January 16, 2024 13:45
Copy link
Collaborator

@hstojic hstojic left a 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

@uri-granta uri-granta merged commit f76bd82 into develop Jan 17, 2024
12 checks passed
@uri-granta uri-granta deleted the uri/rename_deploy_yaml branch January 17, 2024 08:50
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.

None yet

2 participants