Publish sequence update #1065

joshbruce · 2018-02-19T21:19:06Z

Description

Decided to go with preversion instead of anything related t publish...version only update the package.json file, which needs to be committed anyway; so, thinking anything that needs to be updated and part of a commit should be able to tag along.
Created RELEASE.md for instructions on building a release.

This gives us a three step process when we don't include the setup things - and makes us completely within NPM commands:

npm test
npm version [major|minor|patch]
npm publish

Replaces #1064

Review

Submitter

All tests pass (CI should take care of this, once in place). One known failing test.
All lint checks pass (CI should take care of this, once in place). Not in place yet.
Tests
- Test(s) exist to ensure functionality works (if no new tests added, list which tests cover this functionality).
- No tests required for this PR.
Is release:
- Version in package.json has been updated (see RELEASE.md).
- The marked.min.js has been updated; or,
- release does not change library.

Reviewer

??

joshbruce · 2018-02-19T21:21:01Z

Every time I tried to run the tests and the build at the same time didn't work out so well.

UziTech · 2018-02-19T21:28:51Z

We should probably fix the tests first

UziTech · 2018-02-19T21:30:34Z

I think adding tests to preversion would be a good idea. one less step and it prevents accidentally not checking tests

Feder1co5oave · 2018-02-19T21:39:58Z

RELEASE.md

+
+1. **Major:** There is at least one change not deemed backward compatible. While in beta, the major will remain at zero; thereby, alerting consumers to the potentially volatile nature of the package.
+2. **Minor:** There is at least one new feature added to the release. While in beta, the minor will tend to be more analagous to a `major` release. For example, we plan to release `0.4.0` once we have fixed most, if not all, known issues related to the CommonMark and GFM specifications because the architecture changes planned during `0.4.0` will most likely introduce breaking changes.
+3. **Patch:** No breaking changes. Should fix a defect found in a feature. While in beta, the patch will tend to be more analagous to a `minor` release.


Feder1co5oave · 2018-02-19T21:41:03Z

RELEASE.md

+  - `$ npm test` (run tests)
+  - `$ npm version [major|minor|patch]` (updates `package.json` and creates `min` file)
+  - `$ npm publish` (publish to NPM)
+- [ ] Commit changes locally -> Submit PR to `origina/master` -> Merge PR


Feder1co5oave · 2018-02-19T21:41:49Z

package.json

@@ -33,6 +33,7 @@
  "scripts": {
    "test": "node test",
    "bench": "node test --bench",
-    "build": "uglifyjs lib/marked.js -cm  --comments /Copyright/ -o marked.min.js"
+    "build": "uglifyjs lib/marked.js -cm  --comments /Copyright/ -o marked.min.js",
+    "preversion": "npm run build"


I think it's best committing the minified file too

This should be covered by the npm version updating the package.json. Both files need to be committed at the same time.

Feder1co5oave · 2018-02-19T21:42:13Z

RELEASE.md

+- [ ] Fork `markedjs/marked` -> clone the library locally -> Make sure you are on the `master` branch
+- [ ] Create release branch from `master` (`release-##.##.##`)
+  - `$ npm test` (run tests)
+  - `$ npm version [major|minor|patch]` (updates `package.json` and creates `min` file)


possibly add info about committing too?

I tried running the commit from package.json and it didn't seem to work out. That's actually one of the things that led me to the preversion thing. When we up the version, we have to commit the change to package.json version, which included the changed min file when I tested.

Can you be more specific about what you did?

Tried the npm run build && git commit -m 'minify' -- marked.min.js

Could have been something I messed up on my end. Having said that, if we decide to put the commit directly into the chain, might want to reconsider whether it should be part of preversion. npm version has to be run, therefore, package.json will always be updated and need to be committed.

To be clear, I'm a feel a little weird having the commit be automated - could just be me though.

Maybe you had a npm test set up somewhere along the command chain that failed and aborted everything.
Well I like the single commit (package.json + marked.min.js) idea.

joshbruce · 2018-02-19T21:42:59Z

Agreed on fixing the tests - like I say, it keeps crapping out whenever I run them from the script though...not sure if it's because of the failing test - can't remember if I removed it or not to test that theory; assuming that's what we're talking about here. :)

Inspired mainly by: https://github.com/uswds/uswds/blob/develop/RELEASE.md

Feder1co5oave · 2018-02-19T21:46:57Z

Yes it's because of the failing test. The script exits 1 if any test failed.

This branch solves that problem but I wanted to add some more tests to it before merging.

Feder1co5oave · 2018-02-19T22:02:17Z

RELEASE.md

+  - `$ npm test` (run tests)
+  - `$ npm version [major|minor|patch]` (updates `package.json` and creates `min` file)
+  - `$ npm publish` (publish to NPM)
+- [ ] Commit changes locally -> Submit PR to `origin/master` -> Merge PR


make clear to commit package.json and marked.min.js?

See if the added lines cover it.

joshbruce · 2018-02-19T22:37:36Z

Some of the comments from @Feder1co5oave made me think we might benefit from a PR Template for the project...let me know if y'all think it is too much and I'll remove it from this PR.

Feder1co5oave · 2018-02-20T00:46:21Z

I was just thinking... do we really need to open a branch and then submit a PR and then merge it to master just to do a release?

UziTech · 2018-02-20T01:49:27Z

I like that flow. Forcing PRs gives an easy way to comment on changes and makes it easy to review changes. It makes it easier for multiple people to work on the same repo.

+1 on the PR template. We could even have multiple PR templates for different types of PRs

joshbruce · 2018-02-20T12:46:58Z

@Feder1co5oave: Re the branch + PR, I find it brings a consistency to operations - all code changes go through the same process. If I could figure out a way to make it so admins couldn't push, I probably would, to be honest. I like having everyone playing by the same rules...even admins. I shouldn't be able to bypass the review process just because I'm an admin.

To your point, technically, no, we don't have to do all that to create a release. The GitHub release is a professional courtesy. Even committing the new version number to GitHub is optional, I think - could just run $ npm publish then discard changes. Again, for me, it's mainly about having the same process for all the things; also agree with @UziTech's assessment as well. :)

@UziTech: Didn't know multiple templates were a thing. Not sure they've figured out the UX on that score yet (having to update the URL seems odd); however, that could explain why they took submitting PRs out of GitHub Desktop.

joshbruce · 2018-02-20T12:47:58Z

Y'all should be able to merge, if you agree with these changes...let me know if you can't.

Feder1co5oave · 2018-02-20T12:59:46Z

@joshbruce I'm specifically speaking about releasing, which does not bring code changes per se. I agree that any change to the code base should be reviewed before merging.

Feder1co5oave · 2018-02-20T13:02:15Z

Actually I still can't do anything... nothing changed from before the markedjs org

joshbruce · 2018-02-20T13:26:16Z

@Feder1co5oave and @UziTech: Take a look this time. Upgraded both of you to have "write" permissions. master is considered a protected branch; so, it might be a bit funky.

Also, double check the "projects" tab to see if you can play with the task boards, please.

Feder1co5oave · 2018-02-20T13:42:04Z

I can edit projects and issues, but I can't merge:

joshbruce · 2018-02-20T14:19:43Z

@Feder1co5oave: Please try again and let me know. I've never used the PRs need review before merging option, wondering if that's what it was.

Feder1co5oave · 2018-02-20T14:23:00Z

Nope, still like the image above. That prevents merging non-reviewed PRs for everyone.
It seems I don't have write access.

The "authorized" link takes me here

joshbruce · 2018-02-20T14:32:29Z

Thanks! Let's try again. Removed protected status for master - may not have a lot of options. I always see the button because I'm an owner, I think (because I did set the selection to require admins live by the same rules as everyone else and could still merge, if I wanted to).

joshbruce · 2018-02-20T15:43:45Z

Also, here is the cheat sheet I'm working from: https://help.github.com/articles/repository-permission-levels-for-an-organization/

Feder1co5oave · 2018-02-20T16:39:28Z

Alright, I see the Big Green Button!

joshbruce · 2018-02-20T16:49:52Z

@Feder1co5oave: Right on! Having write access seems to have been the ticket. Thinking we should all stick to using forks for now, unless there's a reason to use the mainline itself that I'm missing.

I'm going to try to protect master again. Will try to take it one step at a time.

Feder1co5oave · 2018-02-20T16:50:42Z

Understood!

joshbruce · 2018-02-20T16:56:44Z

In theory:

@UziTech, @Feder1co5oave, and @styfle should be able to merge this PR (big green button): Can you see the merge PR button?
All of our status checks should have to pass...when we turn on Travis, this will include lint and tests.
Only org or repo admins will be able to push directly to Master; thereby, bypassing PR and CI processes, I think - that's why working from forks should probably be a thing. :)
In theory, even admins of the org and repos should have to abide by the same rules; so, we can't circumvent things.
We are not requiring PR reviews at this time prior to merging. Do we want to? (This can add a lot to the mix and may be overkill given where we are at present with the revival.)

Feder1co5oave · 2018-02-20T16:58:09Z

@UziTech, @Feder1co5oave, and @styfle should be able to merge this PR (big green button): Can you see the merge PR button?

Nope.

UziTech · 2018-02-20T17:01:23Z

It is grayed out for me

styfle · 2018-02-20T17:04:31Z

I see grayed out button

This pull request can be automatically merged by project collaborators
Only those with write access to this repository can merge pull requests.

Feder1co5oave · 2018-02-20T17:06:10Z

From the git point of view merging a PR is equivalent to pushing to the branch, so you must enable it for us. I'm not sure about requiring status checks to pass. When we'll enable CI, the status will be clearly visibile right above the merge button so the reviewer can easily check its log but I personally wouldn't put a hard rule on it since it's not very well tested yet.

Feder1co5oave · 2018-02-20T17:07:52Z

On the other hand, requiring at least a positive review looks like a good compromise to me

joshbruce · 2018-02-20T17:13:00Z

@Feder1co5oave: I hear ya. I think from the GitHub user-perspective (permissions), you need to have write access as collaborators to be able to merge (before it was read). So, only collaborators with write access can merge.

Turned off status checks for now...let me know what you get. (Sorry, too accustomed to working alone in organizations - odd as that may sound.)

Feder1co5oave · 2018-02-20T17:17:25Z

Still a grey button here... and here is what I see on a CI-checked PR:

UziTech · 2018-02-20T17:29:29Z

Somewhat unrelated question... should we be added to the markedjs org? Being part of the organization would give us a private discussion area so we wouldn't need to open prs or issues to discuss stuff.

joshbruce · 2018-02-20T19:33:35Z

@UziTech: Invites sent.

Can I also get a green button check? Only options I have set right now are to protect master and apply rules to admins - but there are no other checks marked.

styfle · 2018-02-20T19:39:58Z

LGTM 👍

joshbruce · 2018-02-20T20:42:58Z

All right. Going to merge this one.

Then I'm going to see what happens when I do the sequence to confirm things.

Feder1co5oave · 2018-02-22T17:32:39Z

Sorry I've been a little absent and did a sloppy review, but I feel I have a couple of problems with this proposal.

The pull request template is a little confusing. First of all, all the checklist items regarding the release should not be there, but possibly in the RELEASE.md. They're confusing since they really have nothing to do with a PR that aims to solve some issue.
Plus I've already voiced my opinion that releases don't really need a PR, but I see that having the need for a review can help in making sure everything is okay before releasing. So it is important to submit the release PR, wait for a review, merge the PR, and only then publish on npm and create the release on github. (maybe make this clear in RELEASE.md?)

About the release PR, the things that needs to be checked are:

tests (covered by CI in the PR status)
linting (covered by CI in the PR status)
new version in package.json
marked.min.js is updated. I don't see the point of releasing without a code change unless it is the case only the package.json file need an update (like in Update package.json to new GitHub repo #1054 but that is rare).

So all in all, three things need to be checked regarding the release PR by the reviewer:

CI status is green
package.json has the new version in it
marked.min.js is updated.

Whereas, in case of a contribution PR to solve some issue, I would treat it as if it were an "issue + patch" combination. So it either references some already submitted issue, or it presents the issue itself in the correct manner (expected/actual input/output samples):

specify which version of marked is affected (either by npm version or commit hash you're using)
if you're behind the current master, check that out and see if the issue has already been fixed (if you can)
if relevant, post clear input, expected output, and actual output:
```
## input
```
```
<expected output>
```
```
<actual output>
```
if possible, type the input in babelmark and inspect the output by the different converters (notice that they're using an outdated marked version so don't take its output into consideration)
if an error is thrown, post the complete call stack and the markdown input

Then, for the code change, I would prepare the following checklist:

new tests added (if not necessary, list which tests cover this functionality)
old tests pass (run npm test)
code style is good (run npm run lint and correct any error messages before committing)
if feature proposal, document it clearly in the README

I would leave listing the other fixed issues to us collaborators since that is quite tricky.

So I guess these are my draft proposals for the issue and pull request templates, let me know!

Publish sequence update

Josh Bruce added 2 commits February 19, 2018 14:42

Publish lifecycle

d856fba

Create file for releasing instructions

c6114e8

joshbruce requested review from UziTech and Feder1co5oave February 19, 2018 21:19

joshbruce mentioned this pull request Feb 19, 2018

[WIP] Preparing prepublishOnly #1064

Closed

Josh Bruce added 2 commits February 19, 2018 15:27

Updating checklist

54f5936

Make 3 NPM commands sub-bullets

df8c1fe

Josh Bruce added 2 commits February 19, 2018 15:30

Moving content around a bit

dbf5e43

Ediitng

080d473

Feder1co5oave approved these changes Feb 19, 2018

View reviewed changes

Fix typo

794d944

Feder1co5oave reviewed Feb 19, 2018

View reviewed changes

Josh Bruce added 2 commits February 19, 2018 16:33

Create PR Template and update RELEASE based on feedback

3670bfa

Typos and allowing for no release PRs

ad98c98

joshbruce changed the title ~~[WIP]: Publish sequence update~~ Publish sequence update Feb 20, 2018

joshbruce merged commit e47a584 into markedjs:master Feb 20, 2018

joshbruce deleted the publish-sequence-update branch February 20, 2018 20:43

joshbruce mentioned this pull request Feb 20, 2018

Release 0.3.16 #1067

Closed

11 tasks

joshbruce mentioned this pull request Feb 24, 2018

Update PR Template #1075

Closed

Feder1co5oave mentioned this pull request Feb 27, 2018

Stepping down from developer #1093

Closed

intcreator mentioned this pull request Feb 28, 2018

Auto-linking - cm_autolinks test #1092

Closed

zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021

Merge pull request markedjs#1065 from joshbruce/publish-sequence-update

4d0438c

Publish sequence update

snyk-bot mentioned this pull request Apr 12, 2022

[Snyk] Security upgrade rollup-plugin-license from 2.3.0 to 2.7.0 ekmixon/marked#72

Open

Publish sequence update #1065

Publish sequence update #1065

Conversation

joshbruce commented Feb 19, 2018 • edited Loading

Description

Review

Submitter

Reviewer

joshbruce commented Feb 19, 2018

UziTech commented Feb 19, 2018

UziTech commented Feb 19, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshbruce commented Feb 19, 2018

Feder1co5oave commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshbruce commented Feb 19, 2018

Feder1co5oave commented Feb 20, 2018

UziTech commented Feb 20, 2018

joshbruce commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

joshbruce commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

UziTech commented Feb 20, 2018

styfle commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018 • edited Loading

Feder1co5oave commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 20, 2018

UziTech commented Feb 20, 2018 • edited Loading

joshbruce commented Feb 20, 2018

styfle commented Feb 20, 2018

joshbruce commented Feb 20, 2018

Feder1co5oave commented Feb 22, 2018

joshbruce commented Feb 19, 2018 •

edited

Loading

Feder1co5oave commented Feb 20, 2018 •

edited

Loading

UziTech commented Feb 20, 2018 •

edited

Loading