Downstream only: Rework of Makefile and incusion of lint#339
Conversation
|
|
||
| .PHONY: verify-modules | ||
| verify-modules: | ||
| @echo "verify-modules target is a placeholder to use same targets as upstream CI" |
There was a problem hiding this comment.
how this calls the target in the other Makefile?
There was a problem hiding this comment.
It does not, it's just a placeholder that does nothing, but the make ci target is exactly the same as upstream:
Line 330 in 32e4c95
equals to our Makefile.prow target.
There was a problem hiding this comment.
if it does nothing, while other makefile does anotherthing, it is not the same...
There was a problem hiding this comment.
Ok will update to call original upstream.
There was a problem hiding this comment.
Should be resolved. Please check.
There was a problem hiding this comment.
will try to produce a test locally
|
@mateusoliveira43 while adding lint target I did notice that this PR introduced lint error, because we never ran lint jobs, should I fix it with this PR or address with another. We will unfortunately need to backport this to other branches of downstream Velero: 32e4c95#diff-23f2d480d6500154fe7ea296a25696240962d708378c7d8f4648958e3579a864R34 The linting error: |
|
@mpryc first, the errors needs to show in our CI, so adding it to makefile target is needed We need to confirm the errors are only in our patches, as upstream files should already be linted and we need to find a way to always run the same version as upstream. currently it is the same, but after rebase, it will be outdated https://github.com/openshift/release/blob/master/ci-operator/config/openshift/velero/openshift-velero-konveyor-dev.yaml#L5 |
I don't understand that part? We do have go version only in the above file, so after which rebase it will be outdated? We don't run manually container execution of |
|
wrong link, sorry right one https://github.com/vmware-tanzu/velero/blob/main/hack/build-image/Dockerfile#L97 golangci-lint version is my concern, not container |
WFM :) |
How about we do something smart inside a Makefile and if it's not found then fail: $ grep -oP 'golangci-lint/master/install.sh.*\Kv[0-9]+\.[0-9]+\.[0-9]+' hack/build-image/Dockerfile
v1.57.2 |
| KUBEBUILDER_ASSETS=$(shell echo $(shell $(GOBIN)/setup-envtest use -p path) | sed 's/ /\\ /g') | ||
| .PHONY: envtest | ||
| envtest: $(GOBIN)/setup-envtest | ||
| .PHONY: lint |
There was a problem hiding this comment.
I would remove this
we are adding without using it
There was a problem hiding this comment.
I want to introduce prow job for that target, but first we need to have it in the Makefile so prow CI job can be successful. It's chicken-egg problem.
There was a problem hiding this comment.
on OADP, we have everything under unit-test job
would do the same approach here
There was a problem hiding this comment.
This is very much different from the upstream and usually lint is not part of unit. Having it separate gives clean information that the code is OK, just needs reformatting without checking job logs.
shubham-pampattiwar
force-pushed
the
konveyor-dev
branch
from
32e4c95 to
3fac568
Compare
|
needs rebase |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
done |
|
@mpryc I'm concerned that this is going to lead to rebase merge conflict headaches every time upstream Makefile changes. Not sure what the right answer is, though. |
|
@sseago this is |
|
Corresponding release change: openshift/release#55969 |
|
/retest |
|
I don't like how this pr is showing 41 commits.. did you fetch openshift before rebasing against openshift/konveyor-dev? |
The rework of Makefile to make it more readable and inclusion of lint as a target as well extract golangci-lint version from the upstream Dockerfile, so we test in PROW or locally on the same version as upstream. Signed-off-by: Michal Pryc <mpryc@redhat.com>
mpryc
force-pushed
the
makefile_rework
branch
from
004f596 to
6a7f921
Compare
Fixed, that was based before rebase of velero, that's why. |
|
@mpryc: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaovilai, mpryc, sseago The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cherry-pick oadp-1.4 |
|
@mpryc: new pull request created: #345 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
7 participants
The rework of Makefile to make it more readable and inclusion of lint as a target.
code-generatorwhich was not used in the Makefile.prowcode-generatorverify-modulestarget so nowmake ciis calling exactly same targets as upstreamMakefileTests performed: