search

jefflinse / pr-semver-bump

A GitHub Action to bump and tag a new semantic version when a pull request is merged.
MIT License
24 stars 11 forks source link

add base-branch option to only consider version tags on the PR base branch #25

Closed jtgrohn closed 1 year ago

jtgrohn commented 2 years ago

SUMMARY

Add a base-branch option to only consider version tags on the PR base branch. Default behaviour remains the same - base-branch defaults to false. When set to true, the action filters out tags that are not on the PR base branch to find the latest version tag - as opposed to considering all tags in the repository.

There is no open issue for this change, but the desire is to have pr-semver-bump support hotfix scenarios on release branches for consumers who can't update to latest. Consider the following scenario

v1.1.0 -> v1.1.2 -> v1.2.0 -> v1.2.1

A hotfix needs to be released for v1.1.x, so creating a hotfix branch from the latest v1.1.2

v1.1.0 -> v1.1.2 -> v1.2.0 -> v1.2.1 -> v1.2.2
                  \
                   \-> v1.1.3

The semver in the branch is expected to be auto-bumped to v1.1.3, but pr-semver-bump currently offers v1.2.3. This PR adds a base-branch option so that pr-semver-bump will offer v1.1.3.

The PR hides this functionality behind an option to preserve backwards-compatibility.

It is also behind an option to preserve performance in the default case. There is also a potential performance concern due to all the extra GitHub API querying to find the commits on a branch, as well to find the commit sha a tag is associated with. The older a version a hotfix is requested for, the more quering for tag commit shas will occur. For a sense of scale, it took about ~4.5 minutes for a repo with 31k commits and where it needed to consider < a dozen version tags to find one on the base branch and it took ~40 seconds for a repo with 4.3k commits and < a dozen tags to find one on the base branch.

An alternative approach which I didn't try to code up would be to follow the tree for a branch back to the base branch / tag to find the current version that way. That could theoretically be more performant (assuming in the typical case a branch doesn't have a ton of commits). I don't know if the GitHub API would make that easy though.

RELEASE NOTES

Add base-branch option to only consider version tags on the PR base branch

When base-branch is set to true, the action filters out tags that are not on the PR base branch to find the latest version tag.

jefflinse commented 2 years ago

This is a great feature, thanks for taking the time to put together these changes. I will review shortly.

jefflinse commented 2 years ago

I ran UTs locally and code coverage is still needed for a few code paths in version.js:

------------|---------|----------|---------|---------|-------------------
File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------|---------|----------|---------|---------|-------------------
All files   |   93.04 |    96.97 |   85.71 |   92.86 |
 config.js  |     100 |      100 |     100 |     100 |
 pr.js      |     100 |      100 |     100 |     100 |
 version.js |      80 |    85.71 |   66.67 |   79.49 | 7-20,39,89-90
------------|---------|----------|---------|---------|-------------------
jtgrohn commented 2 years ago

I ran UTs locally and code coverage is still needed for a few code paths in version.js:

------------|---------|----------|---------|---------|-------------------
File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------|---------|----------|---------|---------|-------------------
All files   |   93.04 |    96.97 |   85.71 |   92.86 |
 config.js  |     100 |      100 |     100 |     100 |
 pr.js      |     100 |      100 |     100 |     100 |
 version.js |      80 |    85.71 |   66.67 |   79.49 | 7-20,39,89-90
------------|---------|----------|---------|---------|-------------------

I'm a js noob. Any suggestions for how to test getCommitsOnBranch? Mocking the config object/github api calls like the other tests gets tricky because of the iterator.

I'll get back to this next week - I'm not going to have time this week.

Thanks for the reviews!

Also, I assume you're looking for 100% coverage?

jtgrohn commented 2 years ago

Test coverage should be at 100% now.

jtgrohn commented 2 years ago

Hmm. Looks like this is only actually working within the context of a PR. process.env.GITHUB_BASE_REF isn't picking up the branch for a merge, squash, or rebase commit. I'll get that fixed.

jtgrohn commented 2 years ago

Hmm. Looks like this is only actually working within the context of a PR. process.env.GITHUB_BASE_REF isn't picking up the branch for a merge, squash, or rebase commit. I'll get that fixed.

Fixed with the latest commit

jtgrohn commented 2 years ago

@jefflinse in case it isn't clear, this PR is ready for review :)

jefflinse commented 2 years ago

@jtgrohn Sorry for the delay on this, been a busy few weeks and it fell off my radar. Will re-review this week.