CI improvements #42

msbarry · 2020-12-17T12:37:27Z

Here's a proposal to fix #39. There's a simple job to ensure you can lint, build, and test on windows and mac then separate more thorough targets (from circleci config on 1.13 branch) that run on ubuntu-latest.

At first I thought it was wasteful to do yarn install on each separate jobs running in parallel, but it seems to be pretty heavily optimized (30-40s) so it would actually take longer to use a cache, or upload/download node_modules as an artifact between runs. The downside here is that yarn install result could be slightly different between these runs. For CI this is probably fine though.

After this PR lands:

post comment with artifact snapshot to the PR
fix lint warnings
release workflow (from https://github.com/maplibre/maplibre-gl-js/blob/main/docs/making-a-release.md)

Left to do after #21 lands and we have a map data source to use for development

performance metrics (these require a map data source)
render tests (most render tests use local mvt files, but at least one loads a mapbox:// style url)

msbarry · 2020-12-17T12:39:20Z

@snickell @nyurik any thoughts on this?

msbarry · 2020-12-17T12:41:50Z

.github/workflows/ci.yml

+        uses: actions/setup-node@v2-beta
+        with:
+          node-version: 10
+          architecture: x64


these steps are copy/pasted between each job. I went down a rabbit hole of trying to extract them into composite actions working but couldn't get it working. We could also pull them into sequential steps in fewer jobs if we don't care about running them in parallel?

one thing you may want to consider is introducing some simple shell scripts -- this way you will have as little logic as possible inside the workflows themselves.

I was thinking about this ... you can't factor-out the "checkout", "use node", or "yarn install" steps, so the most promising option looked like moving the "command to run" into the strategy matrix (simplifying multi-step jobs to a single shell script). The problem is they need to produce different artifacts and I couldn't figure out how to get a strategy matrix to bind a variable to multiple values. Any thoughts there? Otherwise I'm inclined to leave as-is for now.

msbarry · 2020-12-17T12:42:59Z

.github/workflows/ci.yml

+      - run: yarn install --frozen-lockfile
+      - run: yarn run test-expressions
+
+  # TODO: need a new data provider for these to run


These need map data to run. We should figure out our plan for what where dev map data comes from for local testing before attempting to re-enable this.

.github/workflows/ci.yml

msbarry · 2020-12-17T12:48:47Z

.github/workflows/ci.yml

+      - run: yarn run test-expressions
+
+  # TODO this is timing-out
+  # collect-stats:


Collecting performance metrics on each change seems important. I'm not sure exactly what this step is doing, but it hangs forever and likely also needs map data.

msbarry · 2020-12-17T12:51:27Z

package.json

-    "test-unit": "build/run-tap --reporter classic --no-coverage test/unit",
-    "test-build": "build/run-tap --no-coverage test/build/**/*.test.js",
-    "test-browser": "build/run-tap --reporter spec --no-coverage test/browser/**/*.test.js",
+    "test-unit": "cross-env build/run-tap --reporter classic --no-coverage test/unit",


Needed to use cross-env here to get these working on windows. Alternatively could use bash which seems a bit less robust, but would eliminate adding any dependencies.

at some point perhaps we ought to make a decision of not targeting building on Windows at all - will make everyone's life easier :)

msbarry · 2020-12-17T13:29:57Z

.github/workflows/ci.yml

+          path: ./test/integration/query-tests
+      - uses: actions/upload-artifact@v2
+        with:
+          path: ./test/integration/query-tests/index.html


Any suggestions on making artifacts from the job easily viewable?

i have had a long discussion with github about this -- you can upload them to artifacts (just like you do here), and use a separate "watcher" job that will be triggered when this job finished. The important difference is that the triggered job (on job completion, or something like that) will run as read-write, having access to the github write token of the maplibre repo. The PR job on the other hand always runs with a read token (because someone malicious could create it). So in the triggered follow up job you can inspect the artifacts and post them to the pull request's comments.

.github/workflows/ci.yml

msbarry · 2020-12-20T12:20:18Z

.github/workflows/analyze.yml

+    types:
+      - completed
+
+jobs:


@nyurik I think I understand github actions permissions a bit better now. I've attempted to implement an action here that reports the bundle size diff in a comment to the PR after the build workflow completes. Does this look reasonable to you? I tested it locally but do you have any ideas on how to test it before landing?

OK I think I got this working - here's an example on a pull request to a separate forked repo: msbarry/maplibre-gl-js-fork#1 (comment)

nyurik

great progress, thanks, left a few minor comments, but i think it is almost ready to merge

.github/workflows/analyze.yml

nyurik · 2020-12-22T16:46:45Z

.github/workflows/analyze.yml

+
+      # Attempt to post a comment with the bundle size report on the PR
+      # Or update the comment on subsequent changes
+      - name: Find Comment


You might find https://github.com/marketplace/actions/sticky-pull-request-comment easier for this usecase -- it simply keeps a hidden comment inside the comment, making it easy to post updates to it with one action. Up to you if you want to use it, but it will keep things simpler IMO

Thanks, I tried using this. The workflow definitely ends up simpler, but it keeps on appending to the previous version of the comment https://github.com/msbarry/maplibre-gl-js-fork/pull/2#issuecomment-749750812 with this config:

- uses: marocchino/sticky-pull-request-comment@v2 with: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} append: false path: comment-body.txt number: ${{ github.event.workflow_run.pull_requests[0].number }}

Any pointers there? Or alternatively, do you think we want it to post new comments/append to the previous comment?

@msbarry I see two edits to that comment. If you look at the raw content of the comment, do you see a hidden comment there with an ID?

Looks like sticky-pull-request-comment doesn't let you set any yaml boolean property to false due to actions/toolkit#361 - I fixed that issue here marocchino/sticky-pull-request-comment#215

BTW, I recommend removing append: false part from settings.

- uses: marocchino/sticky-pull-request-comment@v2 with: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} path: comment-body.txt number: ${{ github.event.workflow_run.pull_requests[0].number }}

Got it! Looks like append will correctly default to false. Thanks.

nyurik · 2020-12-22T16:49:40Z

.github/workflows/ci.yml

+        uses: actions/setup-node@v2-beta
+        with:
+          node-version: 10
+          architecture: x64


one thing you may want to consider is introducing some simple shell scripts -- this way you will have as little logic as possible inside the workflows themselves.

nyurik · 2020-12-22T16:51:28Z

package.json

-    "test-unit": "build/run-tap --reporter classic --no-coverage test/unit",
-    "test-build": "build/run-tap --no-coverage test/build/**/*.test.js",
-    "test-browser": "build/run-tap --reporter spec --no-coverage test/browser/**/*.test.js",
+    "test-unit": "cross-env build/run-tap --reporter classic --no-coverage test/unit",


at some point perhaps we ought to make a decision of not targeting building on Windows at all - will make everyone's life easier :)

nyurik

awesome work, love it!

msbarry added 3 commits December 17, 2020 07:31

CI improvements

be2e953

shorter name

332dc28

shorter

f4d3940

msbarry commented Dec 17, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

collect-stats timing out

4ef9393

msbarry commented Dec 17, 2020

View reviewed changes

undo format change

9c2137c

msbarry commented Dec 17, 2020

View reviewed changes

msbarry added 5 commits December 17, 2020 08:33

name artifacts

1019202

comment-out check-bundle-size, js file needs to be updated

cb3abeb

job timeouts and add browser tests

409f8dd

fix env syntax

ba240e7

fix artifact name

f82bc18

msbarry marked this pull request as ready for review December 18, 2020 12:03

msbarry changed the title ~~WIP: CI improvements~~ CI improvements Dec 18, 2020

msbarry added 6 commits December 19, 2020 05:47

check size

ad69b28

fix condition

71cf735

no if

1579300

no install action

f08dbdf

fix build-script

5a4529e

limit check-size to pull request

5fe2d7b

msbarry commented Dec 19, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

msbarry added 4 commits December 19, 2020 06:15

test bundle size change reporting

f5c5ac0

csp too

4f4bb9c

css too

2ab0c9b

fix

bcab558

msbarry added 9 commits December 19, 2020 06:28

fix superfluous size change

4f96bb0

ci test

1abb989

syntax

b28b3da

branch first

3ae1ad9

repo

916bb94

comment

7435df7

continue on error for parts that may fail

011d7b2

extra quote

0def8f4

run analyze after ci

9b69f38

msbarry commented Dec 20, 2020

View reviewed changes

msbarry added 9 commits December 20, 2020 07:21

retry workflow

f654fc2

rm old workflow

c34c562

change

a4dbc43

fix bundle size reporting

ae75272

remove debug action

f1ff0e3

signed diffs

1d1bf89

include decreases too

f875cd6

update bundle size analysis

618a372

newline

25813b5

nyurik reviewed Dec 22, 2020

View reviewed changes

msbarry added 5 commits December 22, 2020 14:17

analyze.yml

36b0c8e

fix sticky comment

5662caf

master

bd601d4

header

8751f92

remove append=false

7218f92

nyurik approved these changes Dec 23, 2020

View reviewed changes

msbarry merged commit 0a9e564 into maplibre:main Dec 23, 2020

petr-pokorny-1 added this to the 1.14.0 milestone Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI improvements #42

CI improvements #42

msbarry commented Dec 17, 2020 •

edited

Loading

msbarry commented Dec 17, 2020

msbarry Dec 17, 2020

nyurik Dec 22, 2020

msbarry Dec 23, 2020

msbarry Dec 17, 2020

msbarry Dec 17, 2020

msbarry Dec 17, 2020

nyurik Dec 22, 2020

msbarry Dec 17, 2020

nyurik Dec 18, 2020

msbarry Dec 20, 2020

msbarry Dec 20, 2020

nyurik left a comment

nyurik Dec 22, 2020

msbarry Dec 22, 2020

nyurik Dec 22, 2020

msbarry Dec 23, 2020

marocchino Dec 23, 2020

msbarry Dec 23, 2020

nyurik Dec 22, 2020

nyurik Dec 22, 2020

nyurik left a comment

CI improvements #42

CI improvements #42

Conversation

msbarry commented Dec 17, 2020 • edited Loading

msbarry commented Dec 17, 2020

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

Choose a reason for hiding this comment

nyurik left a 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

nyurik left a comment

Choose a reason for hiding this comment

msbarry commented Dec 17, 2020 •

edited

Loading