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

Add --release-draft-only flag #578

Merged
merged 11 commits into from
Dec 26, 2020
Merged

Conversation

Drarig29
Copy link
Contributor

@Drarig29 Drarig29 commented Nov 5, 2020

Closes #478

This is my proposal for the issue above.

  • It adds the argument --release-draft-only to the CLI
  • It adds the function previousTagOrFirstCommit() to get the tag which is before the one which we would have usually used
  • Error if no tag found, which means the package hasn't been published yet
  • Unreleased commits are ignored

Example of use case:

  • First commit
  • Some changes
  • 1.0.0 (tag = v1.0.0)
  • More changes
  • Bug fix
  • 1.0.1 (tag = v1.0.1) (published on NPM, no release on GitHub)
  • One last fix (unreleased)

I do np --release-draft-only.

Behind the scenes, the np command is called with version 1.0.1 "in mind".

The function previousTagOrFirstCommit() returns 1.0.0 so that we can get the commit log from this revision.

The commit which bumps the version to the latest (named "1.0.1") is removed from the history as well as the unreleased commit(s).

No other task is executed. The release draft helper runs right after the first prompt.

@fregante
Copy link
Collaborator

fregante commented Nov 7, 2020

The --release-draft-only flag, while useful, would be the only flag that causes np to not actually publish anything. Should this be a command?

$ np release

Or its own secondary bin command (as part of the same package)

$ np-release

@dopecodez
Copy link
Collaborator

While @fregante does make some valid points, I think that adding a command such as np-release could cause confusion amongst majority of the users, who probably only want a short fix to create a release draft separately.
The way @Drarig29 has implemented this using --release-draft-only should be ok for now, as the option is self explanatory and we can document it to avoid confusion.

@sindresorhus
Copy link
Owner

The --release-draft-only flag, while useful, would be the only flag that causes np to not actually publish anything. Should this be a command?

So does the --preview flag. I think it's fine to have a flag in this specific case.

@sindresorhus
Copy link
Owner

I think this should run right after the first prompt before any of the task stuff. Right now it's just too many conditionals. Would be simpler to just call releaseTaskHelper(options, pkg) as soon as possible.

source/git-util.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add --release-draft-only Add --release-draft-only flag Nov 28, 2020
@Drarig29
Copy link
Contributor Author

Drarig29 commented Dec 2, 2020

Wait, I just found out that the commit range has {lastVersion}...master instead of {lastVersion}...{currentVersion} in it. I need to change that. Actually, no, it's fine. It's just what is in the console output, but the release draft is right.

And I don't know why the checks fail after a timeout... I can't do them locally too.

@dopecodez
Copy link
Collaborator

And I don't know why the checks fail after a timeout... I can't do them locally too.

Don't worry about the failing tests for now, there's a pending PR to fix the integration test. Tests should start passing when its merged.

@sindresorhus
Copy link
Owner

I tried it on https://github.com/sindresorhus/sindre-playground, by adding a commit, but it seems to also include the previous version:

❯ np --release-draft-only

Create a release draft on GitHub for sindre-playground (current: 1.5.4)

Commits:
- dfds  b1b4e37
- 1.5.4  d695405
- sfsf  ee4da19

Commit Range:
v1.5.3...master

Registry:
https://registry.npmjs.org/



 sindre-playground 1.5.4 published 🎉

It should also not show the sindre-playground 1.5.4 published 🎉 message (including the above newlines).

@sindresorhus
Copy link
Owner

The commit range on the opened release draft is also incorrect.

readme.md Outdated Show resolved Hide resolved
@Drarig29
Copy link
Contributor Author

Drarig29 commented Dec 20, 2020

I think you didn't use the option as I intended to.

The use-case scenario I had in mind was that the process of creating a new version had already been done.

This means that the last commit should always be a "tag commit" created by np.

I think you shouldn't be able to see the latest commits in the release draft in the case of this flag, because it does not belong to any release.

What do you think about that?

If you agree with me on the last point, would you prefer:

  1. To have an error (or warning) saying that the current tree has unreleased commits
  2. To show the release draft of the last release (ignoring the latest commits)?

@sindresorhus
Copy link
Owner

Makes sense. I was just not thinking about the original use-case for this.


  1. to have an error (or warning) saying that the current tree has unreleased commits

Just a warning, but still let the user do it if they really want.

@Drarig29
Copy link
Contributor Author

I tried it on https://github.com/sindresorhus/sindre-playground, by adding a commit, but it seems to also include the previous version:

❯ np --release-draft-only

Create a release draft on GitHub for sindre-playground (current: 1.5.4)

Commits:
- dfds  b1b4e37
- 1.5.4  d695405
- sfsf  ee4da19

Commit Range:
v1.5.3...master

Registry:
https://registry.npmjs.org/



 sindre-playground 1.5.4 published 🎉

It should also not show the sindre-playground 1.5.4 published 🎉 message (including the above newlines).

This should be the result now:

❯ np --release-draft-only

Create a release draft on GitHub for sindre-playground (current: 1.5.4)

Commits:
- sfsf  ee4da19

Commit Range:
v1.5.3...v1.5.4

Registry:
https://registry.npmjs.org/

? Unreleased commits found. They won't be included in the release draft, continue? Yes

With those changes, the printed commit log doesn't include unreleased commits, but the warning says "Unreleased commits found". Do you find it's weird? (in fact, they were found but got removed)

I like the fact that it's WYSIWYG, maybe the warning should be phrased differently... how would you say it?

@Drarig29
Copy link
Contributor Author

Drarig29 commented Dec 21, 2020

I think this should run right after the first prompt before any of the task stuff. Right now it's just too many conditionals. Would be simpler to just call releaseTaskHelper(options, pkg) as soon as possible.

By the way, because of this request, the tags aren't pushed to GitHub anymore when we use the --release-draft-only flag (that was the case in my first commits).

So the user might have to do it manually so that the release draft can be bound to the corresponding tag.

@dopecodez
Copy link
Collaborator

By the way, because of this request, the tags aren't pushed to GitHub anymore when we use the --release-draft-only flag (that was the case in my first commits).

I don't think we should proceed with that, pushing the tag and publishing the release is what np does with release drafts in our normal flow. I think having the same behavior for -release-draft-only makes sense.

@Drarig29
Copy link
Contributor Author

The thing is I don't know how I can use the task system again without adding too much conditionals and without copy-pasting the push tags task... if I do it, it might just revert 516c89e

Maybe it's simpler to let the user handle the push tags if it failed, because the two use-cases I think of are the following:

@dopecodez
Copy link
Collaborator

That makes sense, sounds good to me.

@sindresorhus
Copy link
Owner

Also, the flag is named --release-draft-only. I wouldn't expect it to do anything other than presenting a release draft.

@sindresorhus
Copy link
Owner

I released v1.6.0 of https://github.com/sindresorhus/sindre-playground/releases, but I'm still getting the unreleased commit message:

~/dev/oss/sindre-playground master 7s
❯ np --release-draft-only

Create a release draft on GitHub for sindre-playground (current: 1.6.0)

Commits:
- dfds  b1b4e37

Commit Range:
v1.5.4...v1.6.0

Registry:
https://registry.npmjs.org/

? Unreleased commits found. They won't be included in the release draft, continue? (y/N)

source/ui.js Outdated Show resolved Hide resolved
source/ui.js Outdated Show resolved Hide resolved
@Drarig29
Copy link
Contributor Author

I released v1.6.0 of https://github.com/sindresorhus/sindre-playground/releases, but I'm still getting the unreleased commit message:

~/dev/oss/sindre-playground master 7s
❯ np --release-draft-only

Create a release draft on GitHub for sindre-playground (current: 1.6.0)

Commits:
- dfds  b1b4e37

Commit Range:
v1.5.4...v1.6.0

Registry:
https://registry.npmjs.org/

? Unreleased commits found. They won't be included in the release draft, continue? (y/N)

Okay, now you should get this:

❯ np --release-draft-only

Create a release draft on GitHub for sindre-playground (current: 1.6.0)

Commits:
- dfds  b1b4e37

Commit Range:
v1.5.4...master

Registry:
https://registry.npmjs.org/

Note that the commit range will contain the latest tag (instead of master) only if there are unreleased commits.

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.

[feat] create release draft option (e.g. np release-draft <version>) would trigger release page
4 participants