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

Release 0.3.17 (template updates) - ?? #1076

Merged
merged 24 commits into from
Feb 27, 2018
Merged

Release 0.3.17 (template updates) - ?? #1076

merged 24 commits into from
Feb 27, 2018

Conversation

joshbruce
Copy link
Member

@joshbruce joshbruce commented Feb 24, 2018

Description

Submitter

  • $ npm version has been run.
  • Release notes in draft GitHub release are up to date
  • Reviewer checklist is complete.
  • Merge PR.
  • Publish GitHub release using master with correct version number.
  • $ npm publish has been run.
  • Create draft GitHub release to prepare next release.

Note: If merges to master occur after submitting this PR and before running $ npm pubish you should be able to

  1. pull from upstream/master into the branch holding this version,
  2. run $ npm run build to regenerate the min file, and
  3. commit and push the updated changes.

Reviewer

  • Version in package.json has been updated (see RELEASE.md).
  • The marked.min.js has been updated; or,
  • release does not change library.
  • case_insensitive_refs is only failing test (remove once CI is in place and all tests pass).
  • All lint checks pass (remove once CI is in place).

@joshbruce
Copy link
Member Author

@Feder1co5oave: Are we in the forcing merges despite failing tests phase??

@Feder1co5oave
Copy link
Contributor

Feder1co5oave commented Feb 25, 2018

Currently, we are. We haven't been in an "all tests passing" status for a long time. Once we fix that test case, we'll make sure that new PRs don't introduce new failing tests.
You can easily check out what went wrong in CI by clicking the "details" link in the checks section of the PR.

@joshbruce
Copy link
Member Author

Think this now covers all the feedback in #1075 except what's under the "input" header - maybe a different issue or conversation. Taking out the WIP part.

@joshbruce joshbruce changed the title [WIP]: Update templates Update templates Feb 25, 2018
@joshbruce
Copy link
Member Author

joshbruce commented Feb 25, 2018

Note: Leaving in the possibility that the release doesn't actually change the library because we may need to change the README or something without actually changing the code...not sure NPM updates that without publishing a version...could be totally wrong (NPM is definitely different than Packagist).

If we can update the README displayed on NPM without publishing an update to NPM - let me know and will definitely take that part out.

@styfle
Copy link
Member

styfle commented Feb 25, 2018

NPM requires publishing a new version to update the README.

It should be a patch update like with

npm version patch

@joshbruce joshbruce changed the title Update templates Release 0.3.17 (template updates) - ?? Feb 25, 2018
@joshbruce joshbruce mentioned this pull request Feb 25, 2018
11 tasks
@joshbruce
Copy link
Member Author

I suppose I could always rollback on my branch but let's see if we can do it this way.

  1. In the previous sequence publishing to NPM happened before the PR; therefore, the previous Release 0.3.16 #1067 (release 0.3.16) was already published to NPM but not merged before the lint and Travis setup...my bad and hopefully not something we have to worry about moving forward.
  2. Went to fix the merge conflicts on the other PR but a) really can't update anything to that version (see previous); so, no GitHub release for 0.3.16 and b) not sure but it seems GitHub or I lost each other anyway. Therefore, this branch got the update from master.
  3. Added @UziTech's recommendation to automatically commit the minification step...still feels weird to me, at least on this PR because I use GitHub Desktop but I'm cool with it if everyone else is as well.
  4. Accepted the recommendation from @Feder1co5oave on order of operations - starting with version and then PR before doing anything else; therefore, this little hiccup shouldn't happen again.

@Feder1co5oave, @UziTech, and @styfle: Broke and fixed some things.

@joshbruce joshbruce self-assigned this Feb 25, 2018
@joshbruce joshbruce modified the milestone: v1.0.0 - All the nope release Feb 25, 2018
@joshbruce
Copy link
Member Author

[edit]: Should not have that problem again...

We also have bigger fish to fry, I think right now regarding the REDOS thing. The changes being made by this PR, beyond the actual package code, are minor and can be easily changed in future.

@Feder1co5oave
Copy link
Contributor

I really thought it obvious that release PRs should only be about releasing and nothing else. Since it is not, I'm now proposing that they are, and to make this a hard rule to follow.

Releasing to npm just to update template changes that are specific to github is nonsense, because they're used only by github. We shouldn't release unless we've got a real reason to.

Is this supposed to be a draft?

@@ -41,7 +41,7 @@
"bench": "node test --bench",
"lint": "eslint --fix lib/marked.js test/index.js",
"build": "uglifyjs lib/marked.js -cm --comments /Copyright/ -o marked.min.js",
"preversion": "npm run build"
"preversion": "npm run build && (git diff --quiet || git commit -am 'minify')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a recommendation from @UziTech. Basically instead of build then commit min...let NPM + Git do it for us. As far as the details of what Git is actually doing I would have to read the docs.

@styfle
Copy link
Member

styfle commented Feb 27, 2018

We shouldn't release unless we've got a real reason to

I agree with @Feder1co5oave 💯

@joshbruce
Copy link
Member Author

@styfle: I agree with @Feder1co5oave as well regarding releasing when we need to not because of some updates to documentation, which is not what caused this PR to become a release. And we have a reason. I really feel like I'm getting a little beat up over something that I was actually doing to try and help fix a problem. Not only that, but now we have a bunch of reasons to do a release.

@Feder1co5oave: Yes, that's supposed to be a draft - I suppose in my zeal and excitement of getting testing updated, the DOS solution in place, having solid docs up on the board, improving the processes and templates for us, having our first contributor who wasn't the four of us report and fix something, and so on, I accidentally hit publish instead of save draft...again, accidents happen...

Gotta step away for a minute, I think...been a busy day on here.

@joshbruce
Copy link
Member Author

Actually I still see it as a draft release...with all the updates as to why it's a release. And, once we merge in #1083 - we got one more reason to do a release.

screen shot 2018-02-26 at 7 59 11 pm

@styfle
Copy link
Member

styfle commented Feb 27, 2018

@joshbruce
I think the problem we are getting at is that there should be two separate PRs:

  1. Update GitHub issue templates
  2. Bump version and release

To quote @Feder1co5oave

release PRs should only be about releasing and nothing else. Since it is not, I'm now proposing that they are, and to make this a hard rule to follow.

@Feder1co5oave
Copy link
Contributor

@joshbruce I now see it as a draft too. I think the new v0.3.17 tag confused me(?)

@joshbruce
Copy link
Member Author

joshbruce commented Feb 27, 2018

@styfle: Agreed. I accidentally pulled master in a way that brought the release-worthy PR submission from @Feder1co5oave into this branch because I was on it. [edit] I also accidentally merged something before we finished the 0.3.16 release on GitHub, which means we technically could not do one, because it would be out of sync with NPMJS; therefore, updated the process accordingly.

Given the overall state of things - I don't think it's the end of the world. NPM and GitHub releases are two completely different things - and neither one has actually happened yet for 0.3.17.

So, if we review the templates - without getting hung up on how we got here - approve them (which I've been using them all day today anyway).

  1. We can merge in the DOS fix security: fix REDOS vulnerabilities #1083.
  2. Possibly merge in the documentation fixes (to update README and all that on NPM as well, because as was pointed out by @styfle, the only way to do that is to publish a release to NPM).
  3. We can then update this branch again with an updated min.

Then our potential future contributors get updated docs (including the AUTHORS file to start working on governance and community things), we get updated checklists that are cleaner, and even a security vulnerability check to boot.

@Feder1co5oave: Yeah. The tag thing confused me as well. Not sure what's going on there.

ps. Even if we don't like these templates...we can update them in pretty short order. I'm more concerned with Marked ending up on the blacklist again and us sticking so much to process that we can't handle accidents committed by contributors easily...including within the ring of committers.

@joshbruce
Copy link
Member Author

joshbruce commented Feb 27, 2018

We also get the improvements to the testing harness submitted by @UziTech and @davisjam. Yes, they should be two separate PRs. Yes, up to this point, they always have been. Yes, in the future, given the updated checklists, it should be harder to have something like this happen again. [edit] No, I don't think it needs to be a hard and fast rule...I don't do well with hard rules when it comes to processes; there's always an exception that will happen and the rule should not be (or, in this case, doesn't necessarily have to be) followed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RR - refactor & re-engineer Results in an improvement to developers using Marked, or end-users, or both.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants