Cirrus: Fix gate task + make lint|validate#5066
Cirrus: Fix gate task + make lint|validate#5066openshift-merge-robot merged 1 commit intocontainers:masterfrom
Conversation
f17ac9f to
2efe315
Compare
A recent Makefile change (4ec893a) removed a side-effect necessary for 'make validation' to pass under automation. Making things worse, change 12bd7e9 was found upon investigation to always point at the latest upstream HEAD. However, this is rarely a fork-point for pull-requests. Further investigation showed the built-in Cirrus-CI, golang-based git does not obtain sufficient data for the Makefile command `git merge-base HEAD $${DEST_BRANCH:-master}` to function properly (in the context of the gate container). Fix this by customizing the clone operation and slightly adjust the Makefile command to function as intended in the gate container. Also add checks to the validate and lint targets which validate the variable EPOCH_TEST_COMMIT value is never an empty string or whitespace. Signed-off-by: Chris Evich <cevich@redhat.com>
2efe315 to
dbb9d09
Compare
edsantiago
left a comment
There was a problem hiding this comment.
Filing as comment, not blocker. If this is truly urgent, I will accept someone else's LGTM with a promise that these issues will be fixed soon.
|
|
||
| .PHONY: lint | ||
| lint: golangci-lint | ||
| @echo "Linting vs commit '$(call err_if_empty,EPOCH_TEST_COMMIT)'" |
There was a problem hiding this comment.
I don't think EPOCH_TEST_COMMIT is actually used here, is it?
There was a problem hiding this comment.
TBH I'm not sure if it's used by the underlying linting tool or not. I'm not opposed to removing it.
There was a problem hiding this comment.
lint acts on a snapshot, i.e. the current source tree. It should not know or care about git history.
| GO ?= go | ||
| DESTDIR ?= | ||
| EPOCH_TEST_COMMIT ?= $(shell git merge-base HEAD $${DEST_BRANCH:-master}) | ||
| EPOCH_TEST_COMMIT ?= $(shell git merge-base $${DEST_BRANCH:-master} HEAD) |
There was a problem hiding this comment.
What is the etymology of this variable name? Since it doesn't seem to be used anywhere else, could it be called GIT_MERGE_BASE instead?
There was a problem hiding this comment.
My best guess is it's used in other containers/* repos. and that was copy-pasted over here when it was eventually needed. Though arguably a bad name, we might consider keeping it to preserve consistency with the other projects.
There was a problem hiding this comment.
I see it used in buildah also. OK, keep it, I can hold my nose.
| CIRRUS_WORKING_DIR: "/usr/src/libpod" | ||
| GOPATH: "/go" | ||
| GOSRC: "/go/src/github.com/containers/libpod" | ||
| EPOCH_TEST_COMMIT: "${CIRRUS_BASE_SHA}" |
There was a problem hiding this comment.
This now leaves a dangling instance of EPOCH_TEST_COMMIT in a comment in this file; that will confuse future maintainers
There was a problem hiding this comment.
don't worry, both past and future maintainers are already confused. It's a job requirement 😄
joking aside, I think #5039 will make the situation slightly better.
There was a problem hiding this comment.
better meaning: I added some comments
|
/lgtm |
|
/hold cancel |
|
/approve |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #5063