-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Cirrus: Fix gate task + make lint|validate #5066
Cirrus: Fix gate task + make lint|validate #5066
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -146,6 +149,7 @@ endif | |||
|
|||
.PHONY: lint | |||
lint: golangci-lint | |||
@echo "Linting vs commit '$(call err_if_empty,EPOCH_TEST_COMMIT)'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think EPOCH_TEST_COMMIT
is actually used here, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lint acts on a snapshot, i.e. the current source tree. It should not know or care about git history.
@@ -3,7 +3,7 @@ export GOPROXY=https://proxy.golang.org | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it used in buildah also. OK, keep it, I can hold my nose.
@@ -93,7 +93,6 @@ gating_task: | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #5063