Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

Jenkins setup breaks checkcommits #876

Closed
jodh-intel opened this issue Jan 31, 2018 · 8 comments
Closed

Jenkins setup breaks checkcommits #876

jodh-intel opened this issue Jan 31, 2018 · 8 comments
Labels

Comments

@jodh-intel
Copy link
Contributor

.ci/jenkins_job_build.sh is breaking checkcommits by not creating a separate branch for the PR under Jenkins.

See: clearcontainers/jenkins#40.

@grahamwhaley
Copy link
Contributor

So, I understand what you are saying @jodh-intel , and it sounds plausible - but, has checkcommits not been picking up issues - in which case, it feels like there is something I'm missing here. Is this true for all of our repos? Hmm, or maybe it's only been picking up issues on Travis, and not Jenkins?

@jodh-intel
Copy link
Contributor Author

The problem is specific to our Jenkins setup, hence all CC repos I think

Kata is fine as it's using Travis which has a pretty sane git env (which checkcommits detects):

@jodh-intel
Copy link
Contributor Author

Basically, checkcommits requires the PR branch to != the master branch. Or if it does, it needs a way to find the HEAD commit of master (locally).

The question is, if we change the Jenkins config as I've suggested on clearcontainers/jenkins#40, will something else break I wonder? I can raise a test PR, but I'd like to get feedback from @sboeuf and @chavafg too.

@chavafg
Copy link
Contributor

chavafg commented Jan 31, 2018

So this line is the guilty one: https://github.com/clearcontainers/tests/blob/master/.ci/jenkins_job_build.sh#L57
but seems like it was done this way to make coveralls work (as the commit message suggests). @sboeuf do you know if changing it would brake coveralls?

@jodh-intel
Copy link
Contributor Author

@chavafg, @sboeuf - if that's the case, we could always create PR branch, run checkcommits, then merge the PR branch into master before coveralls is run?

@sboeuf
Copy link
Contributor

sboeuf commented Jan 31, 2018

@chavafg @jodh-intel I am not sure about the details here for checkcommits, but please be very careful with this line since this is in charge of the whole sanity of the test. You gotta be careful that we don't end up testing the wrong version or even master in case we actually want to test a PR.

@jodh-intel
Copy link
Contributor Author

Unless someone takes a look today, I'll have a go at doing #876 (comment) tomorrow and see if I can avoid breaking the world.... ;)

@sboeuf
Copy link
Contributor

sboeuf commented Jan 31, 2018

@jodh-intel I don't have time to take a look today, but please let me know tomorrow how your investigations went.

jodh-intel added a commit to jodh-intel/tests that referenced this issue Feb 1, 2018
Rather than merging the PR commits directly into `master`, create a separate
branch for the PR.

This allows `checkcommits` to determine the changes the PR introduces.

After `checkcommits` has run, merge the PR branch back into `master` since
this is required for coveralls.

Fixes clearcontainers#876.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/tests that referenced this issue Feb 1, 2018
Rather than merging the PR commits directly into `master`, create a separate
branch for the PR.

This allows `checkcommits` to determine the changes the PR introduces.

After `checkcommits` has run, merge the PR branch back into `master` since
this is required for coveralls.

Fixes clearcontainers#876.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/tests that referenced this issue Feb 1, 2018
Rather than merging the PR commits directly into `master`, create a separate
branch for the PR.

This allows `checkcommits` to determine the changes the PR introduces.

After `checkcommits` has run, merge the PR branch back into `master` since
this is required for coveralls.

Fixes clearcontainers#876.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/tests that referenced this issue Feb 1, 2018
Rather than merging the PR commits directly into `master`, create a separate
branch for the PR.

This allows `checkcommits` to determine the changes the PR introduces.

After `checkcommits` has run, merge the PR branch back into `master` since
this is required for coveralls.

Fixes clearcontainers#876.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/tests that referenced this issue Feb 2, 2018
Rather than merging the PR commits directly into `master`, create a separate
branch for the PR.

This allows `checkcommits` to determine the changes the PR introduces.

After `checkcommits` has run, merge the PR branch back into `master` since
this is required for coveralls.

Fixes clearcontainers#876.

Signed-off-by: James O. D. Hunt <[email protected]>
mcastelino pushed a commit to mcastelino/tests that referenced this issue Jan 23, 2019
One more tab should be placed for the current_branch variable to fix
broken syntax.

Fixes clearcontainers#876

Signed-off-by: Gabriela Cervantes <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants