-
Notifications
You must be signed in to change notification settings - Fork 204
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
Remove changelog #911
Remove changelog #911
Conversation
Codecov Report
@@ Coverage Diff @@
## master #911 +/- ##
======================================
Coverage 91.8% 91.8%
======================================
Files 142 142
Lines 5699 5699
Branches 885 885
======================================
Hits 5232 5232
Misses 467 467 Continue to review full report at Codecov.
|
This PR will need updating because the API for passing OAuth access tokens to Octokit changed: https://github.com/octokit/rest.js#authentication |
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 like the overall general move toward simplification of client releases. I had some questions...
@@ -9,59 +9,43 @@ | |||
* `v<VERSION>` where <VERSION> is the `version` field in package.json. | |||
*/ | |||
|
|||
const fs = require('fs'); | |||
const request = require('request'); | |||
const Octokit = require('@octokit/rest'); |
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'm usually not quite as pro-dependency, but I like this usage of Octokit
. It makes the code more readable and feels like it will fill in any potential gaps with the way we're communicating with GH. 👍
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 thought so too. They did break their API in a non-major release recently though. Grr...
await octokit.repos.createRelease({ | ||
body: changes, | ||
draft: false, | ||
name: `v${pkg.version}`, |
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.
If I'm grokking this right, this is currently still relying on package.json
's opinion about the current version. This is a piece that would change in step 2 of your proposed path of changes here, yes?
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.
Yes, indeed.
if (!process.env.GITHUB_TOKEN) { | ||
throw new Error('GITHUB_TOKEN env var is not set'); | ||
} | ||
const [owner, repo] = pkg.repository.split('/'); |
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.
The repository
field in package.json
has multiple valid formats/types (https://docs.npmjs.com/files/package.json#repository). I know we're in control of this repo such that we can use the string variant, but even as a string there are a few variants...so I thought I'd point this out.
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.
Good to know. I'd be surprised if anyone but us ends up using this.
@@ -15,8 +15,8 @@ function licenseText() { | |||
return fs.readFileSync('./LICENSE', 'utf-8'); | |||
} | |||
|
|||
function changelogText() { | |||
return fs.readFileSync('./CHANGELOG.md', 'utf-8'); | |||
function codeOfConductText() { |
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.
Can you help me understand how these changes fit in with this set of changes?
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.
The client's dev server used CHANGELOG.md as a dummy document. That's being removed so it needs to render something else instead.
The client deployment process currently involves a commit to master to update the CHANGELOG and version in package.json. This creates a problem when trying to deploy a build that is not the latest release of master. It also creates other failure modes that are not really necessary. As a first step to resolving this problem, remove the changelog in the source tree. The notes attached to the GitHub release will suffice, together with any handwritten updates that developers post in #changelog in Slack. - Remove CHANGELOG.md and scripts to generate it from source tree - Remove changelog references from build process - Change GitHub release script to generate the change list directly instead of reading it from CHANGELOG.md - Change the client's dev server to use the Code of Conduct instead of the changelog as a text source for the test page
Since we're using GitHub's Node REST client already, use it to create the GitHub release instead of making a "manual" HTTP request.
Update script to authenticate using the new API documented at https://github.com/octokit/rest.js#authentication
06c3acc
to
d8a29ce
Compare
The client’s deploy process has some failure modes that only exist because the release process involves a commit to master to update
package.json
andCHANGELOG.md
. As discussed in Slack, we're going to simplify things as follows:package.json
with a dummy version which is only set when the release is actually createdThis PR implements the first step of that plan by removing the changelog file and references to it. The GitHub release script is updated to generate the release notes directly instead of reading from the changelog.