Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cirrus: Fix post-merge env. var. not set. #3601

Merged
merged 1 commit into from
Jul 20, 2019
Merged

Cirrus: Fix post-merge env. var. not set. #3601

merged 1 commit into from
Jul 20, 2019

Conversation

cevich
Copy link
Member

@cevich cevich commented Jul 18, 2019

Signed-off-by: Chris Evich [email protected]

@cevich
Copy link
Member Author

cevich commented Jul 18, 2019 •
edited
Loading

This should fix post-merge testing failing in 'build_each_commit' like this:

git fetch --depth $CIRRUS_CLONE_DEPTH origin $CIRRUS_BASE_BRANCH |& ${TIMESTAMP}
[12:28:57] START - All [+xxxx] lines that follow are relative to right now.
[+0002s] From https://github.com/containers/libpod
[+0002s]  * [new tag]         v1.3.0     -> v1.3.0
[+0002s]  * [new tag]         v1.3.1     -> v1.3.1
[+0002s]  * [new tag]         v1.4.0     -> v1.4.0
[+0002s]  * [new tag]         v1.4.1     -> v1.4.1
[+0002s]  * [new tag]         v1.4.2     -> v1.4.2
[+0002s]  * [new tag]         v1.4.3     -> v1.4.3
[+0002s]  * [new tag]         v1.4.4     -> v1.4.4
[+0002s]  * [new tag]         v1.2.0     -> v1.2.0
[12:28:59] END - [+0002s] total duration since START
make build-all-new-commits GIT_BASE_BRANCH=origin/$CIRRUS_BASE_BRANCH |& ${TIMESTAMP}
[12:28:59] START - All [+xxxx] lines that follow are relative to right now.
[+0000s] # Validate that all the commits build on top of origin/
[+0000s] git rebase origin/ -x make
[+0000s] fatal: invalid upstream 'origin/'
[+0000s] make: *** [Makefile:426: build-all-new-commits] Error 128
[12:28:59] END - [+0000s] total duration since START
Exit status: 2

@mheon
Copy link
Member

mheon commented Jul 18, 2019

LGTM once tests go green

edsantiago
edsantiago reviewed Jul 18, 2019
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alarming, especially with no explanation. Please add a commit message when you have a moment.

(I'm not blocking, because it sounds like this is urgent, but I fervently hope that haste will not lead to further emergencies).

.cirrus.yml Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
@cevich
Copy link
Member Author

cevich commented Jul 18, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2019
@rhatdan
Copy link
Member

rhatdan commented Jul 18, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2019
@cevich
Copy link
Member Author

cevich commented Jul 18, 2019 •
edited
Loading

I'm not blocking, because it sounds like this is urgent, but I fervently hope that haste will not lead to further emergencies.

it is kind of urgent? But maybe not? GCE or Cirrus-CI have been have some random problems, but otherwise testing in PRs seems to be working fine. Post-merge testing has been failing for days on master, and I'll accept partial fault for not noticing 😢 Still, I appreciate you jumping in to take a look. I'll follow up first thing Friday morning.

@cevich cevich force-pushed the fix_post_merge_build_each_commit branch from fe6cb5d to cac66ee Compare July 19, 2019 12:59
@cevich
Copy link
Member Author

cevich commented Jul 19, 2019

Rebased, added comments, added missing fix @edsantiago found.

@cevich
Copy link
Member Author

cevich commented Jul 19, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2019
edsantiago
edsantiago reviewed Jul 19, 2019
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but could I please, please beg you to add a commit message? This code will break in the future, maybe soon or maybe not, and a future maintainer would really appreciate knowing the history behind this change. This is the proper place to document the fact that CIRRUS_BASE_BRANCH cannot be used because it's only set at such-and-such times.

.cirrus.yml Outdated Show resolved Hide resolved
@cevich
Cirrus: Fix post-merge env. var. not set.
4c6e8aa 
Cirrus-CI automatically sets `$CIRRUS_BASE_BRANCH` during PR testing.
This is used for the `build_each_commit` task, in order to compute the
commit-chain properly.  However, prior to this commit and after a PR
merges, the post-merge `build_each_commit` task would fail with
something similar to:

```
make build-all-new-commits GIT_BASE_BRANCH=origin/$CIRRUS_BASE_BRANCH |& ${TIMESTAMP}
[12:28:59] START - All [+xxxx] lines that follow are relative to right now.
[+0000s] # Validate that all the commits build on top of origin/
[+0000s] git rebase origin/ -x make
[+0000s] fatal: invalid upstream 'origin/'
[+0000s] make: *** [Makefile:426: build-all-new-commits] Error 128
[12:28:59] END - [+0000s] total duration since START
Exit status: 2
```

This is because `$CIRRUS_BASE_BRANCH` is undefined when CI runs against
a branch (by design).  This commit fixes the problem by referring to
`$DEST_BRANCH` instead.  This variable must always point at the intended
destination branch for testing, and so can be used in this context as
well.

Also updated a few comments to help steer understanding of the
`$DEST_BRANCH` purpose.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the fix_post_merge_build_each_commit branch from cac66ee to 4c6e8aa Compare July 19, 2019 18:41
@rhatdan
Copy link
Member

rhatdan commented Jul 20, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2019
@openshift-merge-robot openshift-merge-robot merged commit 8364552 into containers:master Jul 20, 2019
@rh-atomic-bot rh-atomic-bot mentioned this pull request Jul 20, 2019
Merged
@cevich cevich deleted the fix_post_merge_build_each_commit branch June 30, 2021 18:11
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edsantiago edsantiago edsantiago left review comments

@giuseppe giuseppe Awaiting requested review from giuseppe

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cevich @mheon @rhatdan @openshift-ci-robot @edsantiago @openshift-merge-robot