New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic support for white-listing tests to run on new PRs. #10

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@samoht
Copy link
Contributor

samoht commented Jun 12, 2017

Currently, the only working option is to say "test this" to allow
a PR to be tested.

Signed-off-by: Thomas Gazagnaire thomas@gazagnaire.org

@samoht

This comment has been minimized.

Copy link
Contributor

samoht commented Jun 12, 2017

/cc @dave-tucker The workflow for testing stuff is very basic atm but it should be able to do the basic things needed, e.g. build/test will be done if someone on the whitelist opened the PR or if someone on the whitelist said "test me". The code compiles but is untested ...

Before merging, a whitelist file should added at the root of this repo, containing GitHub logins (one per line). DataKit will monitor this repo and will recompute PRs "testability" everytime a new commit is done to this repo.

@samoht samoht requested a review from talex5 Jun 12, 2017

@talex5
Copy link
Contributor

talex5 left a comment

I think the whitelist check should be a separate (non-cached) build step. This would avoid duplicating the check in build and test, and should make it rebuild exactly when needed.

let test_this c =
(* The user doing the comment is in the whitelist and he/she
says "test this" *)
in_whitelist (Comment.user c) && contains "test this" (Comment.body c)

This comment has been minimized.

@talex5

talex5 Jun 13, 2017

Contributor

Looks like Jenkins uses test this please - maybe that's less prone to mistakes? (e.g. Did you test this?)

@@ -368,7 +381,7 @@ module Builder = struct
| Error `Not_file -> DK.Tree.read_file tree Cache.Path.value >>*= load_json t
| Error e -> Utils.failf "Unexpected DB error: %a" DK.pp_error e

let branch _t { Key.src; target} =
let branch _t { Key.src; target; _} =

This comment has been minimized.

@talex5

talex5 Jun 13, 2017

Contributor

It doesn't make sense to ignore something that's in the key. If you want all PRs, branches and tags to rebuild whenever the whitelist changes, the (hash of) the whitelist needs to be in the branch name.

If you don't, then the whitelist should be in the context.

Most likely, checking whether someone is whitelisted has nothing to do with building LinuxKit and should be checked as a separate (non-cached) build step. Then, changing the whitelist will only rebuild PRs affected by the change.

Result_cache.find t job_id key
Term.target target >>= fun target_v ->
let key = { Builder.Key. src; target; whitelist; } in
let context = { Builder.job_id; target = target_v } in

This comment has been minimized.

@talex5

talex5 Jun 13, 2017

Contributor

I don't think putting the target in the context makes sense either. Whether a PR is built does depend on the state of the PR, not just on the commit.

@samoht samoht force-pushed the samoht:whitelist branch from ff21537 to 39ab8f4 Jun 13, 2017

@samoht

This comment has been minimized.

Copy link
Contributor

samoht commented Jun 13, 2017

As suggested by @talex5, I've added the PR check as a separate pipeline step so it could be cached more easily. The current pipeline becomes:

  • pull whitelist from linuxkit/linuxkit-ci/master/whitelist
  • check that the target (PR, ref) can be built using the information stored in the whitelist
  • build and test as it is now

@samoht samoht force-pushed the samoht:whitelist branch 3 times, most recently from 80c13b3 to 41e6d9d Jun 13, 2017

@dave-tucker dave-tucker referenced this pull request Jun 19, 2017

Merged

Add Whitelist #11

@samoht samoht force-pushed the samoht:whitelist branch from 41e6d9d to ffdad9e Jun 19, 2017

@dave-tucker

This comment has been minimized.

Copy link
Contributor

dave-tucker commented Jun 21, 2017

LGTM and a whitelist file now exists in this repo. Is this good to merge/test?

@samoht

This comment has been minimized.

Copy link
Contributor

samoht commented Jun 21, 2017

It is using unreleased features in datakit-github, so I need to add a few pins in the Dockerfile (doing that now). But if you managed to get it compiled it should be good to test.

samoht added some commits Jun 13, 2017

Add basic support for white-listing tests to run on new PRs.
Currently, the only working option is to say "ok to test" to allow
a PR to be tested.

Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
Fix Dockerfile
Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>

@samoht samoht force-pushed the samoht:whitelist branch from ffdad9e to 722a2f4 Jun 21, 2017

@samoht

This comment has been minimized.

Copy link
Contributor

samoht commented Jun 21, 2017

I've just fixed the Dockerfile so you should be able to test it more easily.

@samoht

This comment has been minimized.

Copy link
Contributor

samoht commented Aug 6, 2018

What's the current plan regarding LinuxKit CI? Should I close this PR?

@justincormack

This comment has been minimized.

Copy link
Collaborator

justincormack commented Aug 6, 2018

Hmm, we don't have a very determinate plan over CI at present... so maybe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment