feat: support custom release tag in GitHub #431

Apidcloud · 2017-09-15T23:06:58Z

It's just a simple change, but as mentioned in #97, this PR should make GitHub releases use a custom tag whenever defined - just as it happens with npm release.

Does it need anything else? @gr2m said something about resetting the tag. I wonder if that's really needed.

I have noticed the tests (even before the changes) fail though. Seems to be an issue in plugins?

test/specs/plugins.js ................................. 4/8
  export plugins
  not ok Unexpected token function

gr2m · 2017-09-15T23:15:18Z

src/post.js

      var tag = {
        owner: ghRepo[0],
        repo: ghRepo[1],
-        ref: 'refs/tags/v' + pkg.version,


You are replacing creating the v1.2.3 tags. We will not remove that feature. But we could additionally create a tag if a custom tag is set in publishConfig.

Not sure I understand the first part. Shouldn't it match with release.tag_name and release.name?

instead of creating a single tag, we know would create 2 tags at once:

the version tag in the form of v1.2.3

the release tag which matches the name of the tag used to publish the package on npm

Does that make sense?

ah, so this line would be kept in v1.2.3 format and only the release one changes if publishConfig is present?

nothing changes, if we release a new version 1.2.3 to npm, we will still create a git tag v1.2.3. But if publishConfig.tag is to set "next" then we can create an additional git tag called next.

What I’m not exactly sure about is what happens if a git tag next already exists from an earlier release. We would probably check if it does, if yes delete it, then create it again.

So I just need to create an additional reference (github.gitdata.createReference) when publishConfig.tag is present, and the release (github.repos.createRelease) shouldn't be changed, right?

gr2m · 2017-09-15T23:15:42Z

test/specs/post.js

@@ -9,6 +9,9 @@ var post = proxyquire('../../src/post', {

 var pkg = {
  version: '1.0.0',
+  publishConfig: {
+    tag: 'beta'
+  },


Don’t change an existing test, please create a new on

I was about to, but it raises the error I mentioned even without the changes. What do you recommend to fix that?

the tests should pass, they just did about 19h ago on travis:
https://travis-ci.org/semantic-release/semantic-release/builds/275739048

The tests that currently fail are related to your changes.

Before the changes, what errors do you see?

Hmmm that's weird. I forked and just ran npm install and npm test. Do I need anything else?

Log: https://pastebin.com/HARMnXg5

I've installed it, and it's running in http://localhost:5984/_utils/ but I'm not sure what else I need to set it up for the tests. couchdb doesn't work in the terminal despite being added (manually) to the path environment variable (windows).

The whole setup is not ideal right now, I don’t even know if it’s working on Windows :(

as a workaround, can you try to adapt these lines for local testing:

semantic-release/test/registry/start.sh

Lines 22 to 23 in 88b5895

COUCH=http://admin:password@127.0.0.1:15986

couchdb -b -a local.ini -p couch/pid -o couch/stdout.log -e couch/stderr.log

Change the port number in L22 to to 5984 and comment out the second. Make sure you created an admin account with username "admin" and password "password" at http://localhost:5984/_utils/

Still doesn't seem to work. I will try with a different couchdb version tomorrow, as running couchdb is producing some weird output (kernel-poll not supported). Probably some windows black magic going on here 💃

I’m sorry for the trouble, thanks for your patience! If it doesn’t work, you can still push and have travis run the integration tests.

It seems to be a problem with privileges in Windows... Still trying to figure out a solution.

Either way please check my other comment, related to the tags 👍

Apidcloud · 2017-09-18T11:00:29Z

Please check the changes 👍 I'm still not sure how to test it thoroughly though.

gr2m · 2017-09-18T17:04:34Z

I also don’t know yet, but I’ll have a look. I’ll be busy with work until Saturday, I'll have a look then.

What you can do until then is to setup a test repository for a test npm package, set "semantic-release": "Apidcloud/semantic-release#GitHubCustomTag" in your package.json so your fork gets installed, and then see if it works, if there are any problems left you’ll catch it that way

Apidcloud · 2017-09-22T00:46:47Z

After some struggle, I think I got it. It's deleting and creating a new one when it already exists.
You can check the builds here: https://github.com/Apidcloud/node-fusionchart-demo
Or just https://travis-ci.org/Apidcloud/node-fusionchart-demo

Please also give it a try and let me know what you think.

pvdlg · 2017-10-03T00:10:03Z

@Apidcloud thanks for the contribution!

We now use the git tags as a fallback to determine the commit that was used to make the last release. And we expect the tag associated with the release to be named according npm recommendation (or at to the way npm version creates it).

In addition, it's not possible to have a version released multiple time on different tags.
So if you release 2.0.0 on the @next diest-tag you won't be able to release 2.0.0 on the latest dist-tag. You would have to run npm dist-tag add <pkg>@2.0.0 latest to add the latest dist-tag to version 2.0.0. See dist-tag.

That means that when you release 2.0.0 with the next dist-tag and create the git tag tags/v2.0.0 (as it's done in this PR, or maybe you intended to do next/v2.0.0 ?) at some point v2.0.0 will be made available on the latest dist-tag and the git tag won't make much sense anymore. It will just reflect that originally v2.0.0 was released as a beta, even though it's now available on latest.
Not all the versions you release on next are meant to become available on latest as some might have bugs, but some are, and they will have a inconsistent tag name.

What would be the objective of your proposal? Could you describe which scenario you'd like to achieve? Thanks !

pvdlg · 2017-09-25T03:11:19Z

test/specs/post.js

+    }, function (err, published, release) {
+      tt.error(err)
+      tt.is(published, true)
+      tt.match(release, defaultRelease)


Should you test that the custom tag has been created ?
Because defaultReleasetag_name is: 'v1.0.0' which is not the custom tag. So if the code works and actuallt create a custom tag, this test would fail.

Yeah, I wasn't sure how to test it. Can give it another try though.

pvdlg · 2017-09-25T03:18:56Z

src/post.js

+              }
+            })
+          }
+        })


What is the purpose of this section of code?
It seems outside of the scope of this PR (which is to customize git tag based on release tag)?

It might be a good candidate for another PR. Even though we would have to think about the consequences of deleting tags. Maybe the user has other tags, maybe there is other things relying on this tag etc...So it's probably safer to log the error and explain the user what to do rather than taking the risk of deleting something the user might care about.

This part makes sure the reference (tag) doesn't exist when we create it, otherwise, it throws an error. But on a first glance, I believe I forgot to check whether the custom tag was defined in the first place.

Isn't the github API would already throw an error when you create a tag that already exist?

Exactly, and thus deleting it when that's the case. There might be better ways to do so, though.

What I mean is:

We probably don't want to delete tags. We have no way to check if the original tag is intentional or not. We don't want to take any destructive action without user consent.

If the objective is to test if the tag exist before creating it, the "create tag" API call is already doing that.

I agree with you. Originally I thought the same tag could have multiple versions within, but I believe that's not possible and thus the need of replacing it.

Would next/v2.0.0 as you mentioned earlier make more sense?

Apidcloud · 2017-10-03T01:13:24Z

The first idea was to mirror what happens over npm when releasing under a custom tag (say alpha).

But as the GitHub behaviour is rather different, the purpose of this PR is to create a 2nd tag (when defined) that points to the same release. Doesn't have to be next.

Let me know your thoughts!
Thanks!

pvdlg · 2017-10-03T01:19:29Z

The first idea was to mirror what happens over npm when releasing under a custom tag (say alpha).

I might be wrong but I think npm create a tag like v2.0.0 when you do npm version <patch|minor|major> and doesn't create a tag when doing npm publish --tag next.
So it seems that what happen with npm is that the git tag is not related in any way to the dist-tag.

But as the GitHub behaviour is rather different, the purpose of this PR is to create a 2nd tag (when defined) that points to the same release. Doesn't have to be next.

It didn't occur to me that the code was creating a second tag, but I checked fairly quickly :)
What's the point of creating a second tag that points to the same version? Can you describe your workflow or how that would be useful ?

Apidcloud · 2017-10-03T01:31:02Z

TLDR:
I guess what I wanted was the ability to do the release under an alpha or beta tag (dist-tag) just like what happens over npm.

Ok. I'll try to explain it based on what I wanted (and still want) to accomplish.
I'm developing some packages that aren't ready for production use. With the current semantic-release version, when it comes to npm releases, I'm able to do it under a beta or alpha tag.

In GitHub the behaviour is different. There's just a numerical version (e.g., v0.4.0) as explained in the issue #97 by @gr2m:

but only npm gets the beta/next/custom tag

What I originally wanted was to mirror what happens in npm, so I suggested mixing the GitHub tag with the custom tag (e.g., v1.0.0-alpha) and make the release with that. But I think that's not acceptable for you, is it?

Then the idea was to create a 2nd tag (custom tag, only when defined) reference that points to the same numerical release.

Does that make sense? I'm not sure myself anymore.

pvdlg · 2017-10-03T02:10:11Z

I understand. But as I was trying to explain in this comment the notion of git tag and npm dist-tag are quite different.

A git tag mark a commit as a milestone in the history and is not meant to be changed.
An npm dist-tag is a distribution channel (Arguably the term dist-channel would be a lot less confusing). It works like the Chrome dist channel:

Version 2.0.0 is release (it's the only version 2.0.0 that will ever be released and the commit associated with 2.0.0 will never change)
This version is cutting edge, has many new features and might have bugs, so it's made available only on the distribution channel next (or dist-tag in npm world). Therefore only early adopters (aka adventurous users) will use it because they opt in by setting the dependency as 'my-package@next'.
If everything goes well and it turns out nobody report bugs on v2.0.0 for a month or so, we decide to also make it available to regular users by doing npm dist-tag add my-package@2.0.0 latest. Version 2.0.0 will now be distributed to everyone. But 2.0.0 is still the same, point to the same commit and the same git tag.

In that scenario there is no version 2.0.0-beta and version 2.0.0-stable. There is only 2.0.0 distributed to a subset of users for a month then distributed to everyone.

So it doesn't make much sense to have a git tag, meant to represent a milestone in the evolution of the package, to be named after it's temporary distribution channel.

The way github handle that is with pre-release. See creating-releases, especially:

If the release is unstable, select This is a pre-release to notify users that it's not ready for production.

What would make more sense would be to create github release flagged as pre-release at first (when your package is distributed only on next channel/dist-tag).
And when/if you are ready to make that release available to everyone you would do npm dist-tag add my-package@2.0.0 latest and go to the github release and uncheck the pre-release.

In terms of semantic-release you would configure your package.json like this:

 "publishConfig": {
    "tag": "next"
 },
"release": {
  "pre-release": true //New option we would have to add
}

Each time semantic-release would make a release it would be:

Available only on the npm next distribution channel
Flagged as pre-release on Github

If the version is never deemed good enough for general availability, it would stay like that. But if it is deemed good enough for general availability you can move it to the npm lastest distribution channel and remove the pre-release flag on github.

Does that make sense? Would that work for you?

pvdlg · 2017-10-03T02:13:53Z

Here is a good blog post on the subject: https://blog.greenkeeper.io/one-simple-trick-for-javascript-package-maintainers-to-avoid-breaking-their-user-s-software-and-to-6edf06dc5617

pvdlg · 2017-10-03T02:24:26Z

In addition, it's worth noting that npm does support the prereleases but as mentioned here semantic-release doesn't recommends to use them and recommends to use dist-tags instead, fitting the scenario I explained above.

Apidcloud · 2017-10-03T02:43:55Z

Thank you for the wonderful explanation and link. I think I understand it better now.

Would the migration to latest dist channel be done manually though? And should this PR focus on creating a pre release in GitHub when the dist tag is next and the pre release is set to true? (Is this flag even needed? Shouldn’t it happen by default as the tag is next?)

Thanks again! I’ll try to take a look tomorrow.

pvdlg · 2017-10-03T03:19:09Z

Would the migration to latest dist channel be done manually though?

I think so, because semantic-release wouldn't have a way to determine which release can be moved to the latest channel nor when to do so.

And should this PR focus on creating a pre release in GitHub when the dist tag is next and the pre release is set to true?

If that fit your need and you would like to make a PR for this feature, yes. But maybe it would be better to start a new PR (referencing this on in the comment). Also it's probably better to do that independently of the release branch configured in "release": {"branch": "branch-to-release-from"}.

Is this flag even needed? Shouldn’t it happen by default as the tag is next?

Some users might name their beta/unstable/preview/notProdReadyYet branch other than next. In addition some users might want to not use the Github pre-release feature at all independently of the distribution channel they choose. So it's probably better to keep it configurable.

@gr2m do you have some feedback / concerns on this ?

gr2m · 2017-10-03T04:16:20Z

And should this PR focus on creating a pre release in GitHub when the dist tag is next and the pre release is set to true?

The only problem I see with this is that when you set the @latest dist-tag on npm to the new version, you should have to manually update the release on GitHub, too, as it no longer is a pre release.

Is this flag even needed? Shouldn’t it happen by default as the tag is next?

I’m undecided. I’d be okay flagging a GitHub release as "pre-release" if the dist tag is set to anything else than latest, but we should inform folks about that in the docs and tell them that they have to remove the flag once they set the @latest dist-tag on npm.

pvdlg · 2017-10-03T04:35:36Z

The only problem I see with this is that when you set the @latest dist-tag on npm to the new version, you should have to manually update the release on GitHub, too, as it no longer is a pre release.

Yes indeed. I'm fiddling with what we can do regarding the refactoring of the cli (the main semantic-release not the setup one) to have only one command (instead of pre > publish > post and the reliance on 1 exit code). Maybe we could add the possibility to run manually semantic-release --latest <version> that would do npm dist-tag add <package>@<version> latest and remove the pre-release flag on github at the same time.

I’m undecided. I’d be okay flagging a GitHub release as "pre-release" if the dist tag is set to anything else than latest, but we should inform folks about that in the docs and tell them that they have to remove the flag once they set the @latest dist-tag on npm.

We also have to handle situations where folks publish on a legacy dist channel in order to do bug fixes on old versions. For example one can maintain a legacy version that retain node 4 compatibility, and release only bug fixes there. So the package users could reference <package>@legacy and receive only the latest bug fixes on the node 4 compatible legacy version.
That's probably a very rare situation though. But maybe if we do a Github pre-release by default on any publish that is not on latest dist-tag, we should have a way to override that behavior for some specific dist-tags.

gr2m · 2017-10-03T04:53:00Z

Maybe we could add the possibility to run manually semantic-release --latest that would do npm dist-tag add @ latest and remove the pre-release flag on github at the same time.

That’d be super nice!

I’m also thinking about the GitHub app that could do such things, but that’s out of scope and would only work for GitHub. Love the idea to make it work with the CLI!

gr2m · 2017-10-03T04:54:42Z

We also have to handle situations where folks publish on a legacy dist channel in order to do bug fixes on old versions. For example one can maintain a legacy version that retain node 4 compatibility, and release only bug fixes there

Good point ... we could also add another option the package.json publishConfig ... or be smart about it and see if the released version is higher than the currently @latest, and only then set the GitHub release to be a pre-release?

pvdlg · 2017-10-03T04:55:33Z

I’m also thinking about the GitHub app that could do such things, but that’s out of scope and would only work for GitHub. Love the idea to make it work with the CLI!

I was thinking the same :) But that would require a user action (as the decision to make a release available on the latest channel can only be made by a human). Therefore a UI. Therefore quite some work (at least for someone with no design talent like me :-))

pvdlg · 2017-10-03T05:11:21Z

Good point ... we could also add another option the package.json publishConfig ... or be smart about it and see if the released version is higher than the currently @latest, and only then set the GitHub release to be a pre-release?

Yes, but we are touching a complex subject. I've been thinking about that and hoe deal with or prevent situations of "non consistent" release across dist-tag.

For example if folks have a legacy, latest and next dist-tags.
Last release on legacy is 1.1.0, last release on latest is 2.0.0, last release on next is 2.1.0.
What if you make a breaking change commit on the legacy channel ? => Fails because 2.0.0 already exists.
What if you make a breaking change commit on latest? Release 3.0.0, but the last release on latest is higher than last release on next. I think npm would handle that without any issue but seems weird. Or not. I'm not sure.

I have a few ideas on how to handle those cases but the deeper I dig the more edges case I find and the more unlikely they get. My best shot so far would be to have the last-release-npm plugin to also report the versions already released in other tags or unpublished (as npm doesn't allow to publish a version on an unpublished one) and to skip those versions when we determine the next release version number.

Anyway that's not really the subject of this PR. But maybe it's safer to keep it configurable, and try later to improve the algorithm to automatically determine the appropriate config?

Apidcloud · 2017-10-03T14:11:01Z

It might be better to create another PR then. And just to make sure, it would address the following:

A custom tag other than latest + pre-release flag mark the GitHub release as pre-release.

Does that sound ok?

pvdlg · 2017-10-03T14:22:40Z

A custom tag other than latest+ pre-release flag mark the GitHub release as pre-release.
Does that sound ok?

The idea is to mark the github release as a pre-release if the dist-tag in publishConfig is not latest by default. But to add a possibility to override that with a config that would allow to define other branches on which we want a non pre-release release.

Maybe an option with that can accept a string or an array of string that defines the branches the branches on which we don't do a non pre-release. If this config is not present, then we consider it's ['latest'] by default.

Keep in mind that in order to accept the PR we would ask to have unit tests that cover all the cases introduced by the change. I refactored all the unit test recently so they should be easier to understand, but feel free to ask any questions :)

Apidcloud · 2017-10-03T14:51:08Z

Ok. Where would that branch config be placed? Inside publishConfig?

pvdlg · 2017-10-03T15:10:27Z

I'd say in release as it's semantic-release specific and publishConfig is npm specific.

gr2m · 2017-10-03T16:05:59Z

What if you make a breaking change commit on the legacy channel ? => Fails because 2.0.0 already exists.

Well you don’t, this would be like an LTS version, you keep maintaining it, but you can’t do breaking changes. We had discussion on this somewhere else. The idea is that when I release 3.0.0 I’d create a branch 2.x.x and in that branch I could set a base version to "2" so releases from that version would release as the next version within 2.x.x, but it would not allow breaking changes at all. Not sure what the npm tag would be, we could default to v2 or we could require a dist-tag setting that isn’t "latest".

pvdlg · 2017-10-03T16:15:48Z

Well you don’t, this would be like an LTS version, you keep maintaining it, but you can’t do breaking changes.

Yep. Enforcing this kind of things is not something we do currently. But we probably should. We would need to know the precedence of dist tags though (next > latest > legacy). The verifyRelease plugin might be a good candidate for that. And a GitHub app would allow us to handle the situation better by opening an issue or even better, setting the status of the PR that would introduce a non allowed breaking change.

Apidcloud · 2017-10-03T19:21:52Z

Maybe an option with that can accept a string or an array of string that defines the branches the branches on which we don't do a non pre-release. If this config is not present, then we consider it's ['latest'] by default.

Shouldn't it be ['master'] by default? I mean, shouldn't we compare it to the branch name instead of the dist tag? Not sure since you mentioned 'branch'.
Also, what do you think of naming the flag no-pre-release instead?

pvdlg · 2017-10-03T20:04:59Z

My mistake, I said branch but I meant dist-tag. We are checking the dist-tag not the branch to determine if we do a github pre-release or not. So the default should be ['latest'].

Ok for no-pre-release, I can't find a better name.

Apidcloud · 2017-10-03T22:19:17Z

I've just created another PR. I believe this one might be closed.

pvdlg · 2017-10-03T23:50:45Z

Thanks so much ! I'll review shortly

gr2m requested changes Sep 15, 2017

View reviewed changes

Apidcloud force-pushed the pr/GitHubCustomTag branch from d8733e6 to 3d1e5af Compare September 18, 2017 10:41

Apidcloud force-pushed the pr/GitHubCustomTag branch 3 times, most recently from 6525197 to 3dea014 Compare September 22, 2017 01:40

feat: support custom (additional) tag in GitHub release

a8e0090

Apidcloud force-pushed the pr/GitHubCustomTag branch from 3dea014 to a8e0090 Compare September 22, 2017 01:55

pvdlg mentioned this pull request Sep 25, 2017

Populate gitHead, and use Git tag if it's not there #393

Closed

pvdlg added the info requested label Oct 3, 2017

pvdlg requested changes Oct 3, 2017

View reviewed changes

Apidcloud mentioned this pull request Oct 3, 2017

feat: allow pre-releases on GitHub #457

Closed

pvdlg removed the info requested label Oct 3, 2017

pvdlg closed this Oct 3, 2017

pvdlg mentioned this pull request Oct 4, 2017

Make git tag name configurable #409

Closed

pvdlg mentioned this pull request Nov 1, 2017

Pre-release discussion #207

Closed

	COUCH=http://admin:password@127.0.0.1:15986
	couchdb -b -a local.ini -p couch/pid -o couch/stdout.log -e couch/stderr.log

feat: support custom release tag in GitHub #431

feat: support custom release tag in GitHub #431

Conversation

Apidcloud commented Sep 15, 2017 • edited

gr2m Sep 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apidcloud Sep 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apidcloud Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apidcloud Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apidcloud Sep 16, 2017 • edited

Choose a reason for hiding this comment

Apidcloud commented Sep 18, 2017

gr2m commented Sep 18, 2017

Apidcloud commented Sep 22, 2017 • edited

pvdlg commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apidcloud Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apidcloud commented Oct 3, 2017

pvdlg commented Oct 3, 2017

Apidcloud commented Oct 3, 2017 • edited

pvdlg commented Oct 3, 2017 • edited

pvdlg commented Oct 3, 2017

pvdlg commented Oct 3, 2017

Apidcloud commented Oct 3, 2017

pvdlg commented Oct 3, 2017

gr2m commented Oct 3, 2017

pvdlg commented Oct 3, 2017

gr2m commented Oct 3, 2017

gr2m commented Oct 3, 2017

pvdlg commented Oct 3, 2017

pvdlg commented Oct 3, 2017

Apidcloud commented Oct 3, 2017

pvdlg commented Oct 3, 2017

Apidcloud commented Oct 3, 2017

pvdlg commented Oct 3, 2017

gr2m commented Oct 3, 2017

pvdlg commented Oct 3, 2017

Apidcloud commented Oct 3, 2017 • edited

pvdlg commented Oct 3, 2017

Apidcloud commented Oct 3, 2017

pvdlg commented Oct 3, 2017

Apidcloud commented Sep 15, 2017 •

edited

gr2m Sep 15, 2017 •

edited

Apidcloud Sep 17, 2017 •

edited

Apidcloud Sep 16, 2017 •

edited

Apidcloud Sep 16, 2017 •

edited

Apidcloud Sep 16, 2017 •

edited

Apidcloud commented Sep 22, 2017 •

edited

Apidcloud Oct 3, 2017 •

edited

Apidcloud commented Oct 3, 2017 •

edited

pvdlg commented Oct 3, 2017 •

edited

Apidcloud commented Oct 3, 2017 •

edited