-
Notifications
You must be signed in to change notification settings - Fork 80
chore: Automate adding clog to manifest #208
Conversation
|
✨ Coder.com for PR #208 deployed! It will be updated on every commit.
|
This comment has been minimized.
This comment has been minimized.
d77fa5e to
ae0403f
Compare
|
The CI failures for running |
| "lint:fix": "markdownlint --fix '**/*.md'" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/minimist": "^1.2.1", |
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.
@vapurrmaid Opening as a reply here so that we can have a threaded discussion
The CI failures for running next build should disappear after some time - I imagine the m/ version pulled in is cached/takes time to pull in changes or the node_modules install step is cached on CI.
If there's something wrong with CI and caching, we should fix so that we can trust it. Otherwise we can end up in a situation where merging this might cause coder.com or dashboard builds to start failing?
I think the problem might be that the monorepo root package.json needs these dependencies added, or perhaps the one in product/coder.com/site? The preview CI clones the monorepo and then updates the docs repo, but runs the build from the coder.com site root, so my guess is that one needs to be updated to make nextjs happy
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.
This build is kinda janky because we can't actually build docs independently (they are built as a subpage of the full site)... so I think we need to add these deps to https://github.com/cdr/m/blob/bdd808f0bd10252503b8cee86b71bf329d579f85/product/coder.com/site/package.json#L11 as a separate PR first, get that merged into m, then the previews should hopefully work and will be safe to merge?
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.
@jawnsy I appreciate the thread and confidence in CI - I don't oversee what/how node_modules are cached, but am confident it's related to that due to the test below which I performed manually in latest off mainline in m/:
$ cd product/coder.com/site/docs/
$ git checkout origin/vapurrmaid/manifest
Previous HEAD position was 6cd3673 Add 1.17.1 changelog (#206)
HEAD is now at 820897d chore: Do not use semis
$ yarn
$ cd ..
$ yarn buildWhich succeeds
Otherwise we can end up in a situation where merging this might cause coder.com or dashboard builds to start failing?
This is only true if we bump this submodule in m/ which will also need to pass CI
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.
Did you also go up to coder.com and run the yarn build from there?
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.
Nevermind, misread your session. Well alright!
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.
It has a further advantage of allowing Coder users to download a zip of our documentation locked to a specific 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.
We're not currently using dependabot to track npm modules in m, but yeah that's certainly a possibility.
The fact that coder.com/site is parsing a ts file in cdr/docs is weird to me.
Agree with you there, all of this is weird and kinda unexpected, ha.
Looking at the Vercel dashboard, it shows stuff checked out at the monorepo level, though I guess we have customized vercelignore to exclude irrelevant stuff like the product:
But this does mean that it should be running postinstall from the root, so I'm not sure why the dependencies are missing?
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.
it should be running postinstall from the root, so I'm not sure why the dependencies are missing?
My original theory is it would take some time to be able to pull the latest m/ commit containing d318b93a1c414c988d0b3951b3f96aeaf65e6306 which originally failed CI in m/.
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.
However, that seems debunked as https://github.com/cdr/m/commit/a6223b278c3a44713bd5ca4bdfad9a8df674fa1a was pulled in on last run which is after https://github.com/cdr/m/commit/d318b93a1c414c988d0b3951b3f96aeaf65e6306
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.
Well, every build is pulling the latest monorepo code at the start of the preview build: https://github.com/cdr/docs/blob/6ec7c2313588fdfafea6a67121c60f98f8c0efb8/.github/workflows/preview.yaml#L14-L21
820897d to
4e6f769
Compare
Context: In c886756 a clog generation script was added in order to help automate changelog generation. However, generated clogs still needed to be manually added to manifest.json. At the time, use of jq was experimented with in the create-clog.sh script, but was found to be not worth the effort. Summary: Adds a NodeJS script written in TS for adding changelog entries to manifest.json. Details: The manifest.json file was formatted as part of this diff because the add_clog_to_manifest.ts script uses JSON.stringify to serialize changes to JSON. The resulting format differed from our prior format, but still complies with prettier.
4e6f769 to
67ff178
Compare

Context
In c886756 a clog generation script was added in order to help automate changelog generation. However, generated clogs still needed to be manually added to
manifest.json. At the time, use ofjqwas experimented with in thecreate-clog.shscript, but was found to be not worth the effort.Summary
Adds a NodeJS script written in TS for adding changelog entries to
manifest.json.Details
The
manifest.jsonfile was formatted as part of this diff because theadd_clog_to_manifest.tsscript usesJSON.stringifyto serialize changes to JSON. The resulting format differed from our prior format, but still complies with prettier.Example