jumpstart / git-caching

[allisonkarlitskaya]

The summary:

• minor changes to the copyright scanning script to avoid a mess on stderr
• add a new git-utils.sh which contains a common implementation of the logic for maintaining and using a cache of a git repository of a given name relative to GITHUB_BASE, with utilities for checking it out to various different places in the cockpit tree
• use that from tools/node-modules
• use that from tools/make-bots
• use that from test/common/pixel-tests
• use that from a new script tools/jumpstart
• use that last one from test/make_dist.py
• only commit dist/ and package-lock,json to cockpit-dist

[martinpitt]

Thanks! Aside from the naming question (hardest of all!) my biggest worry is about the split. The rest is small local issues which should hopefully be easy to fix.

[allisonkarlitskaya]

the pixel tests commit was a disaster, so I've dropped it.

[martinpitt]

packit fails with

tools/jumpstart: line 29: NODE_ENV: unbound variable

I'm ack'ing github-changes here, mostly want to see it being removed by the next force-push again 😁

[allisonkarlitskaya]

I'm ack'ing github-changes here, mostly want to see it being removed by the next force-push again grin

Uh. Oops?

The deconstruction of the context object in the script is bugged. There is no context.number, but rather context.issue.number. I'm gonna get rid of the entire thing and replace it with direct references in the call. I start to find this deconstruction assignment syntax to be a bit troublesome... That's for another PR, though.

[martinpitt]

The fine-grained stuff is fine now, thanks for the fixes! Now we just need to talk about the outstanding issue above (not having two scripts which do almost the same thing, and defining its purpose). "script that does something" is not good enough, sorry 😁

[allisonkarlitskaya]

So there are two separate scripts here, that actually don't have a lot to do with each other, in my opinion.

One script takes a fresh repository and pre-seeds it with some nice stuff to get you up and running as fast as possible. It's very logically called "jumpstart", because that's what it does.

The other script builds a tarball from a repository, using the best means available (possibly including #1, but not always) and tells you the path to the tarball. It's very logically called "make dist", because that's what it does.

Let's chat tomorrow :)

[martinpitt]

How about tools/webpack-jumpstart? That aligns with tools/webpack-{make,watch} and at least says what it jumpstarts. Maybe that's the sort of good compromise that makes us about equally (but hopefully not too much) grumpy 😁

[allisonkarlitskaya]

I'm fine with the name, if it needs to be that way (although it pushes against my plans to also add a bots checkout).

The only real issue remaining here is what to do with make_dist.py. You mentioned that packit is using that (and some quick grepping also turns up livetest.yml and make-debs/make-srpm), so I'm afraid we can't just merge it into image-prepare. I'm happy to port it to shell if you think that's nicer, and maybe move it to tools/. I'm also happy to move --wait to the jumpstart script...

Thoughts?

[allisonkarlitskaya]

As discussed, the latest changes:

• script is now called webpack-jumpstart
• drop the publish workflow changes. will proposed those in a separate PR along with a change to use the newly-added pkg/build (and no container) to drive the "build" side.

[martinpitt]

Adding blocked, as @allisonkarlitskaya told me on IRC to hold off on this. Also, this needs integration test runs.

[martinpitt]

Thanks! Found a few more little issues, but nothing important -- ok for me to land as-is once you are happy. I'll trigger a few tests over night to get some results.

[martinpitt]

I think this should say dist/. As you noticed today, there's dist/guide which isn't a webpack, and the corresponding workflow is also called build-dist, not build-webpack. I wouldn't mind to rename the script accordingly as well (dist-jumpstart, or download-dist), but I feel for this PR I've already used up my quota of name bikeshedding; I can certainly live with the current one 😁

previous issues got solved

[martinpitt]

The other test failures are known flakes, but this one looks relevant:

fatal: Not a valid object name 380ae06319b3e0ebeba0b46e8117e137e3f66315:.package.json
cmp: EOF on - which is empty

*** node_modules 380ae06319b3e0ebeba0b46e8117e137e3f66315 doesn't match our package.json
*** refusing to automatically check out node_modules

[allisonkarlitskaya]

The other test failures are known flakes, but this one looks relevant:

I have absolutely no idea why that would be the case, particularly considering the build, running in the exact same tasks container, is working for all of the other images.

Maybe some weird race? Ugh.

[allisonkarlitskaya]

The other test failures are known flakes, but [this one looks relevant](https://logs.cockpit-

Pretty sure I figured it out, and it was cool: an interrupted (or concurrently running) git fetch can leave us in a state where we have the commit object present in the repository, but not the tree and file objects that it points to.

Performing a simple lookup with rev-parse, as we do to avoid fetching, would return "true" in that case, but we wouldn't actually be able to access the files.

I'm pretty sure one of our runners ended up with its cache corrupted in exactly this way, which is why this test kept failing, but only for some attempts.

I moved the check over to a full connectivity-checking fsck call instead. That should catch this issue. In that case we'll redo the fetch.

[martinpitt]

Thanks! Green, and the misleading comment above is not that important. I'm sure that we'll touch this stuff more 😁