ci: move pre-commit checks to CI #893

manzt · 2023-05-05T23:39:43Z

Change List

move schema generation to a separate script (scripts/generate-schemas.mjs)

Checklist

Ensure the PR works with all demos on the online editor
Unit tests added or updated
Examples added or updated
Documentation updated (e.g., added API functions)
Screenshots for visual changes (e.g., new encoding support or UI change on Editor)

manzt · 2023-05-06T15:32:44Z

Hmm, can't quite figure it out but now when I run yarn changelog locally, there are no changes (but it is working in CI).

manzt · 2023-05-06T15:35:45Z

.github/workflows/ci.yml

+ node-version: 16
+ cache: 'yarn'
+ - run: |
+ yarn install


I think ideally this action can run very quickly, and fail early (to prevent other jobs from running).

Maybe we could use npx here (rather than yarn installing everything for the repo).

manzt · 2023-05-06T19:05:02Z

scripts/generate-schemas.mjs

+const generator = tsj.createGenerator({
+ path: path.resolve(__dirname, "../src/index.ts"),
+ tsconfig: path.resolve(__dirname, "../tsconfig.json"),
+ skipTypeCheck: true,
+ encodeRefs: false,
+});


Running ts-json-schema-generator from the command line multiple times is unnecessarily expensive, since it needs to run TS and parse the AST each time for the whole Gosling code base. We can re-use the same schema generator for each of our schemas in this single script.

manzt · 2023-05-06T21:48:03Z

Ah, actually I see what's going on. We needed to use fetch-depth: 0 with the actions/checkout to get the whole git history in order for conventional-changelog to work. It relies on the previous tags to understand what need to be written to the CHANGELOG.md.

Everything is working as it should now, it's just this action is extremely slow because:

It needs to checkout the entire gosling history to correctly run conventional-changelog, and the repo is massive (1.37GB) Git repo size #650
It needs to install all of the yarn dependencies for gosling.js to run these commands (really it just needs contentional-changelog and ts-json-schema-generator)

In theory, this "Lint Repo" job should be extremely fast and fail early since it's so minimal, but as it stands it takes the longest out of all the other jobs (including tests and building the editor).

Not sure how to proceed because although this is kind of nicer than pre-commit hooks, its very costly in CI.

EDIT: I guess 2.5 minutes isn't horrible, but reducing the size of the repo and making the yarn install much faster would probably make the job take seconds.

manzt · 2023-05-06T21:54:27Z

package.json

@@ -155,7 +153,6 @@
 },
 "husky": {
 "hooks": {
- "pre-commit": "run-p changelog schema schema-higlass schema-theme schema-template format && git add .",
 "commit-msg": "commitlint -E HUSKY_GIT_PARAMS"


Also, since our "git flow" is squash & merge PRs, idk if it makes sense to lint every commit for conventional commits.

Instead, something like a GitHub action to lint the PR title would probably make more sense

Not sure if either of you have thoughts on this: @etowahadams @sehilyi

It would make sense to use the semantic-pull-request you shared and remove the commitlint we use for every commit.

We were using another semantic PR which is deprecated and does not work anymore. We were using commitlit to handle a case where there is only one commit in a PR because in this case GitHub used the commit message and ignored the PR title. But, using PR title by default is now supported, so I don't think we need commitlint anymore.

I agree! No need to have every conventional commit name be linted.

manzt · 2023-05-06T21:58:39Z

.github/workflows/release.yml

@@ -26,9 +26,11 @@ jobs:
 runs-on: ubuntu-latest
 steps:
 - uses: actions/checkout@v3
+ with:
+ fetch-depth: 0
 - uses: actions/setup-node@v3
 with:
 node-version: 16
 - run: npx changelogithub


I saw this action was added in #881. This action also won't work unless you set fetch-depth: 0.

Ah, got it! Thanks for finding this

sehilyi · 2023-05-06T23:28:31Z

Regarding the costly CI, would the CI speed be more reasonable if we remove conventional-changelog from the CI? Since I think we can check conventional-changelog only at the release and not in every PR

sehilyi

I am open to merging this unless this will likely surpass the available usage of GitHub actions.

manzt · 2023-05-11T21:21:00Z

I don't think we are likely to surpass available usage of GitHub actions (famous last words), but it does mean that currently it will take CI longer to pass (which is annoying for PRs).

I like the idea of only running the contentional-changelog in the release action. However, we then need to make sure the CHANGELOG.md is checked into the repo. Right now it will just be generated in the action and never added with git commit.

manzt · 2023-05-11T21:52:08Z

Should be all good to merge. The main change now is that when you bump the version npm version patch|minor|major, the package.json "version" script will run, executing conventional-changelog and checking in the updated CHANGELOG.md.

The release "flow" is as described in the conventional-changelog docs: https:/conventional-changelog/conventional-changelog/blob/master/packages/conventional-changelog-cli/README.md#with-npm-version

concretely,

npm version patch
git push --follow-tags

manzt added 3 commits May 5, 2023 19:39

ci: try changelog and schema checks in CI

b980629

chore: remove auto git add

ffe4ec2

ci: update workflow name

14ab46b

manzt requested a review from sehilyi May 6, 2023 15:23

manzt commented May 6, 2023

View reviewed changes

manzt added 2 commits May 6, 2023 14:59

refactor: create schema generation script

24dcfe8

chore: add CLI comment

838efe1

manzt commented May 6, 2023

View reviewed changes

manzt added 6 commits May 6, 2023 17:20

ci: force color and cancel early if possible

c02189b

fix: temp disable cache?

8287a4f

ci: re-enable caching

de37658

ci: remove concurrency group

adae6bc

chore: set fetch depth of 0

1394cbf

chore: tmp disable cache

497942a

manzt commented May 6, 2023

View reviewed changes

ci: set fetch-depth 0 for release action

9326848

manzt commented May 6, 2023

View reviewed changes

sehilyi approved these changes May 11, 2023

View reviewed changes

manzt added 2 commits May 11, 2023 17:28

ci: remove changelog from CI workflow

f8eb000

ci: check schemas in linting

2f31efe

manzt force-pushed the manzt/pre-commit branch from 770d0c4 to 2f31efe Compare May 11, 2023 21:41

manzt added 3 commits May 11, 2023 17:43

chore: run changelog in versioning

233c64b

chore: remove unused changelog script

77dc984

chore: remove commitlint and husky

cbe5048

manzt changed the title ~~ci: try changelog and schema checks in CI~~ ci: move pre-commit checks to CI May 11, 2023

ci: fix git typo

282ac52

manzt merged commit 75f4233 into master May 11, 2023

manzt deleted the manzt/pre-commit branch May 11, 2023 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: move pre-commit checks to CI #893

ci: move pre-commit checks to CI #893

manzt commented May 5, 2023 •

edited

Loading

manzt commented May 6, 2023

manzt May 6, 2023 •

edited

Loading

manzt May 6, 2023 •

edited

Loading

manzt commented May 6, 2023 •

edited

Loading

manzt May 6, 2023 •

edited

Loading

sehilyi May 6, 2023

etowahadams May 7, 2023

manzt May 6, 2023

sehilyi May 6, 2023

sehilyi commented May 6, 2023 •

edited

Loading

sehilyi left a comment

manzt commented May 11, 2023

manzt commented May 11, 2023 •

edited

Loading

ci: move pre-commit checks to CI #893

ci: move pre-commit checks to CI #893

Conversation

manzt commented May 5, 2023 • edited Loading

Change List

Checklist

manzt commented May 6, 2023

manzt May 6, 2023 • edited Loading

Choose a reason for hiding this comment

manzt May 6, 2023 • edited Loading

Choose a reason for hiding this comment

manzt commented May 6, 2023 • edited Loading

manzt May 6, 2023 • edited Loading

Choose a reason for hiding this comment

sehilyi May 6, 2023

Choose a reason for hiding this comment

etowahadams May 7, 2023

Choose a reason for hiding this comment

manzt May 6, 2023

Choose a reason for hiding this comment

sehilyi May 6, 2023

Choose a reason for hiding this comment

sehilyi commented May 6, 2023 • edited Loading

sehilyi left a comment

Choose a reason for hiding this comment

manzt commented May 11, 2023

manzt commented May 11, 2023 • edited Loading

manzt commented May 5, 2023 •

edited

Loading

manzt May 6, 2023 •

edited

Loading

manzt May 6, 2023 •

edited

Loading

manzt commented May 6, 2023 •

edited

Loading

manzt May 6, 2023 •

edited

Loading

sehilyi commented May 6, 2023 •

edited

Loading

manzt commented May 11, 2023 •

edited

Loading