flywheel-io / exchange Goto Github PK

It's surprising when a PR fails because the docker build has not yet completed. It would probably be more surprising, and frustrating, for an external contributor, because they can't restart the build on their own. Both @jenreiter and I independently ran into this one.

If the builder cannot find a gear, it should check the registry to see if there's a queued build matching that tag, and poll the registry to wait for completion. My proposal would be to wait for a half hour, printing wait statements once a minute, and fail with a clear message on timeout.

There should never be duplication in the manifests, because that opens the possibility for that information to be wrong. Right now, the version string is duplicated in version and custom.docker-image.

I propose that if custom.docker-image does not contain a colon, it is presumed to use a docker tag named the same as the version field. @jenreiter further proposed making that mandatory - not allowing a colon or version in custom.docker-image at all - but I'd be fine with either approach.

https://circleci.com/blog/sunsetting-1-0/

Validating unchanged botiques/gears impacts performance, and also can cause PRs to fail validation when docker images for unrelated gears become unavailable.

CircleCI ref : https://circleci.com/docs/fork-pr-builds/

The sensitive variables are only required for the actual gear build post-merge, so we may be able to do this without risk after some tweaks. CircleCI allows enabling building of forked PRs, and not exposing secrets configured for the project within CircleCI.

1. Don't fail the script when CI vars are not defined or empty, if we're just validating on a PR -- https://github.com/flywheel-io/exchange/blob/master/bin/process-manifests.sh#L34-L37
2. Don't rely solely on the branch name to process vs. validate, as the source of the fork may come from the master branch. -- https://github.com/flywheel-io/exchange/blob/master/bin/process-manifests.sh#L224
3. Need to confirm if github deploy key is exposed. CircleCI docs are vague on exposure there when a deploy key has been provided.

This ticket was spurred by a PR from a fork, which I believe, is a workflow we want to encourage. #26

When invalid docker images are specified in the gear manifest, the CI script for PRs doesn't catch that, resulting in errors after the merge.

Multiple gears with the same org/name and version number should not be allowed.

• Check for match in master during verification, and fail if found
• Race condition? Need to re-trigger CI for other PRs after merge, and check other PRs/branches?
• Remove duplicates that already exist

We have this content elsewhere. Suggest adding it to the README on this repo, or linking to it.

To discuss: For documentation purposes we can parse the exchange.json file and write an html file, which can can be rendered within the web docs (i.e., confluence). This exchange.html file can be stored alongside the exchange.json file up on gc, or elsewhere.

This does not necessarily have to happen within this repo, but maybe it's cheap enough.