-
Notifications
You must be signed in to change notification settings - Fork 208
Create new plugin : one-release-commit #2180
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
base: main
Are you sure you want to change the base?
Conversation
6c46da3
to
3f0f4b2
Compare
77bc527
to
ce747e0
Compare
ce747e0
to
393e844
Compare
@hipstersmoothie here an implementation proposal to handle #1949 |
@hipstersmoothie anything that I can do to help merge this PR ? |
7e7ab35
to
640fe0a
Compare
@hipstersmoothie I also rebase this PR ;-P |
640fe0a
to
26a4342
Compare
Same as #2182 (comment) |
@hipstersmoothie Let me know if everything miss to merge this 🎉 |
First step create a release context (from release, to, newVersion) Second step use this information to really make the release
26a4342
to
d325b20
Compare
@hipstersmoothie Let me know if this plugin can be merged or if I have something to change, Thanks |
@jBouyoud sorry! been reviewing this and keep getting pulled away. will try to get in ttoday |
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 think we can revert all the changes to core and still have a working plugin
.circleci/config.yml
Outdated
@@ -78,7 +78,7 @@ jobs: | |||
at: ~/auto | |||
- run: | |||
name: Check for SemVer Label | |||
command: yarn auto pr-check --url $CIRCLE_BUILD_URL | |||
command: yarn auto pr-check -vv --url $CIRCLE_BUILD_URL |
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.
Revert this
auto.config.ts
Outdated
@@ -32,6 +32,7 @@ const brewOptions: IBrewPluginOptions = { | |||
export default function rc(): AutoRc { | |||
return { | |||
plugins: [ | |||
"one-release-commit", |
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.
revert this
@@ -0,0 +1,29 @@ | |||
# One-Release-Commit Plugin | |||
|
|||
Allow to create a single release commit |
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.
Write a better description of why someone might want to use this plugin.
|
||
if (commits.length > 0) { | ||
await execPromise("git", ["reset", "--soft", remoteHead]); | ||
await execPromise("git", ["commit", "-m", this.options.commitMessage || `"Release version ${version} [skip ci]"`, "--no-verify"]); |
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.
Make sure to include skip-ci even if there is a custom message
@@ -1760,8 +1775,12 @@ export default class Auto { | |||
dryRun: options.dryRun, | |||
quiet: options.quiet, | |||
}); | |||
|
|||
// New new version must be computed, so make release context | |||
const releaseArgs = await this.makeReleaseContext(); |
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.
This is problematic I think. For the default usage this would included the commit for the changelog in the release notes since it would now be in the commitsInRelease
. Could you make sure this doesn't augment the normal auto workflow?
The changes below to for makeRelaeaseContext
make sense and are good additions but the change in order has me worried.
const commits = await auto.git.getGitLog(remoteHead); | ||
const tags: ITag[] = (await Promise.all(commits.map(commit => getTag(commit.hash)))).filter(tag => tag !== undefined) as ITag[]; | ||
|
||
auto.logger.log.info(`Rewrote ${commits.length} release commits into a single commit for version [${version}] with tags: [${tags.map(tag => tag.name).join(", ")}]`); |
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.
Wait you only need the version for these logs? I don't think that a big enough reason to change core at all.
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.
This log also seems a little early. Nothing happens until the if statement body
this.logger.verbose.info("Calling after version hook"); | ||
await this.hooks.afterVersion.promise({ dryRun: options.dryRun }); | ||
await this.hooks.afterVersion.promise({ dryRun: options.dryRun, version: releaseArgs.newVersion }); |
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.
OH I see why you do all of this rearranging. in the afterVersion hook if it's not a dry run it IS a release. We don't have to care about the return value of makeRelease. The undefined that it returns is for it's other usage. Here we know a release (and thus a version) has happened.
You can revert all the changes to core and remove references in you plugin to version and it still all works!
Create a new plugin to handle this discussion
What Changed
refactor(core): split release creation in two steps
Refactoring needed for the next commit to allow forward current releasing version in
afterVersion
hook.feat(core): add version in
afterVersion
hookAdd
version
inafterVersion
hookfeat(one-release-commit): create plugin with automation
Create plugin boiler plate with automation
feat(one-release-commit): first implementation
First implementation of this plugin
Why
Some users might want a single atomic commit that represent the release (also easy to rollback).
Change Type
Indicate the type of change your pull request is:
documentation
patch
minor
major
🐤 Download canary assets:
auto-linux--canary.2180.26196.gz
auto-macos--canary.2180.26196.gz
auto-win.exe--canary.2180.26196.gz
📦 Published PR as canary version:
under canary scope @[email protected]
✨ Test out this PR locally via: