opensafely-core / cohort-extractor

Cohort extractor tool which can generate dummy data, or real data against OpenSAFELY-compliant research databases
Other
38 stars 13 forks source link

Semantic prefixes in PR titles don't trigger a release #242

Open evansd opened 4 years ago

evansd commented 4 years ago

By pre-pending "fix:" to this PR title, the PR passed the semantic-pull-requests check.

However when merged it didn't then trigger a new release:

Analyzing commit: Merge pull request #240 from opensafely/sqlcmd-improvements
fix: Refactor sqlcmd wrapper for better error handling
The commit should not trigger a release
Analyzing commit: Be slightly more defensive against unexpected output
The commit should not trigger a release
Analyzing commit: Refactor sqlcmd wrapper for better error handling
Previously this code used the `-o output_file` option. This caused both
query output and warnings/errors to be mixed together in a single file,
which then required hairy and error prone code to try to separate back
out.

We now just let sqlcmd write to stdout, which we redirect to a file
ourselves, leaving error output written to stderr.

Closes #239
The commit should not trigger a release
Analyzing commit: Test that SQL runtime errors are handled correctly
The commit should not trigger a release
Analyzing commit: Compare contents of files directly in tests
This makes the test failure output more useful as it shows the diff
between the files.
The commit should not trigger a release
Analysis of 5 commits complete: no release

I think this is because the merge commit looks like this and doesn't contain "fix:" as its first characters:

Merge pull request #240 from opensafely/sqlcmd-improvements
fix: Refactor sqlcmd wrapper for better error handling

Ideally the PR check and the action would agree about what constitutes an appropriately annotated PR.

sebbacon commented 4 years ago

Yes, github-tag-action considers only the first characters of commits.

I see that the conventional commits specification is that every commit must have a prefix, so I think the fix would be to make semantic-pull-requests observe this specification. This has the advantage of consistency, and already being supported in the packages we're using.

However, I can also imagine there not being consensus on this so we should discuss!

I also note that the semantic-pull-requests is actually designed with this CI tool in mind, which is targetted at node apps but I've just noted can be configured for other setups. Given the release tool I implemented already had a pretty egregious bug I had to fix, that might be worth considering....

StevenMaude commented 1 year ago

The semantic-pull-requests GitHub app has long been deprecated, so we're no longer using it here anyway.

But the original issue still stands, so I renamed the issue. The issue is with github-tag-action which still doesn't support PR titles, only the start of commit messages.