Makefile,*: cleanup and refactoring, support reproducible tests locally #4023

joelanford · 2020-10-13T16:34:20Z

Description of the change:
Refactoring and cleanup of the Makefile and hack scripts

Motivation for the change:
The e2e and integration tests are now runnable locally. Overall reduction of complexity and spaghetti.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

~~Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)~~
Add or update relevant sections of the docs website in website/content/en/docs

joelanford · 2020-10-13T16:35:20Z

@camilamacedo86 no rush, but when you get a chance, can you try out each of the make targets in this PR to see if everything works as expected in macOS?

joelanford · 2020-10-13T16:35:40Z

Successful build in my fork is here: https://travis-ci.org/github/joelanford/operator-sdk/builds/735424396

internal/testutils/utils.go

test/e2e-ansible/e2e_ansible_cluster_test.go

camilamacedo86 · 2020-10-13T18:04:15Z

test/e2e-go/e2e_go_suite_test.go

+		Expect(tc.LoadImageToKindClusterWithName("quay.io/operator-framework/scorecard-test:dev")).To(Succeed())
+		Expect(tc.LoadImageToKindClusterWithName("quay.io/operator-framework/custom-scorecard-tests:dev")).To(Succeed())


Could not the name of the images be a const in the operator-sdk/internal/testutils/scorecard.go?

It certainly could. I could go either way.

If we leave it as is, there would be less indirection in the test code, which would make it easier to comprehend and debug, but if we change the scorecard image name (though we probably can't until 2.x), then we'd have to change this string in multiple places.

If we introduce a constant in a separate package, we decrease duplication and centralize the definition of the scorecard image names used in the e2e tests, but we add indirection and make the code slightly more difficult to directly read and understand.

In general, I would usually choose option 2, but I'm leaning toward 1 in this case for two reasons: the image names are extremely unlikely to change for the duration of 1.x and it's nice for tests to have as little indirection as possible so that it is clear what the expectations of the test are.

But I'm good either way. If there's another vote for using constants, I'll switch it over.

A 3. option (my vote) would be a func such as LoadScorercardDevImages which would centralize all. Then, this func would try to load all images and not panic (may just log) when the image X is not found to address the ansible/helm scenarios where is not required both.

.gitignore

Makefile

camilamacedo86 · 2020-10-14T03:30:20Z

Makefile

+	./hack/check-error-log-msg-format.sh
+	go run ./hack/generate/changelog/gen-changelog.go -validate-only
+	go vet ./...
+	./tools/scripts/fetch golangci-lint 1.31.0 && ./tools/bin/golangci-lint run


Could we have the make lint and make lint-fix as well?
IHMO many times we do not need to check all and it is very common to run only make lint and would be make lint-fix which I believe that was removed by mistake to automatic perform the required fixes.

With this PR, I'm intentionally trying to decrease the number of make targets down to a much more manageable set with just the ones that are truly necessary.

IMO make lint isn't much easier or better than just directly running ./tools/bin/golangci-lint run. Personally, I never used make lint and always just ran make test-sanity.

At the end of the day it comes down to what other contributors want, so if there's more support for keeping lint and/or lint-fix around, we can.

With this PR, I'm intentionally trying to decrease the number of make targets down to a much more manageable set with just the ones that are truly necessary.

I am totally all in to reduce the number of targets. However, I think we need to keep some targets as helpers for the developers/contributors.

IMO make lint isn't much easier or better than just directly running ./tools/bin/golangci-lint run. Personally, I never used make lint and always just ran make test-sanity.

So, I'd vote to keep them since (IHMO):

the test-sanity do many checks

I have the impression that contributors have more difficult to understand why the sanity logs and know why the test is not working at all.

and mainly, because the test-sanity requires that we have all committed to passing on that which make harder work with and solve the lint issues. I do not want need to commit to check if has or not lint issues.

the make lint and make lint-fix are easier to remember than ./tools/bin/golangci-lint run and ./tools/bin/golangci-lint run --fix

it would keep the same standard adopted in the KB, controller-runtime and operator-lib.

However, I let it to you folks.

My typical approach to submitting PRs is to make all the necessary changes and commits, and then run through the tests locally before submitting the PR.

When I follow this approach, I am effectively running ALL of the same tests that run in CI. If we stick with each individual thing being its own make target, we end up with (IMO) two negative outcomes:

There would be so many make targets that contributors don't really know what they should or shouldn't use.

Contributors will be less likely to run the full set of CI tests locally before submitting a PR, taking up more CI cycles unnecessarily.

My goal with this PR is to make that process easier and more foolproof. This PR:

Decreases the number of targets to make it easier for contributors to know what's what (the make targets now generally align with the phases of the CI run). Now contributors will have less guessing to do to know what to run prior to submitting a PR.

Makes the e2e tests run in an isolated kind cluster that is used only for operator-sdk e2e tests. Now contributors can run e2e tests in the same way no matter what their local environment looks like.

IMO the benefits far outweigh the drawbacks.

tools/scripts/fetch

camilamacedo86

Just a few nits/comments.
Otherwise, it shows great for me 👍

.golangci.yml

Makefile

estroz

Looks great. Quite a few more make targets will be removed in #4034

/lgtm

internal/testutils/utils.go

Makefile

openshift-ci-robot · 2020-10-15T15:50:59Z

New changes are detected. LGTM label has been removed.

Makefile

estroz · 2020-10-15T22:46:54Z

It looks like the way travis does checkouts may cause a recent tag (from a release) that isn't in a PR's branch to not be present on git fetch origin --tags because the commit itself isn't in the branch's history. This wasn't a problem before we started generating samples, but now that we are, the git version is incorrect unless your branch has that commit. It's fairly easy to fix (rebase your branch onto the trunk); this will also be fixed if we hard-code versions into the Makefile, as planned.

camilamacedo86

IHMO : It is missing :

have a target to gen the samples make samples (we need a target to gen the samples). In POV we could have the make generate which would run make install and gen the docs + samples.
call make install when we run make generate - calling the build instead
have a target to run just the lint and add the lint-fix to the new target fix (it has been discussed)

camilamacedo86 · 2020-10-19T12:47:56Z

Makefile

+.PHONY: fix
+fix: ## Fixup files in the repo.
+	go mod tidy
+	go fmt ./...


WDYT about to add here the lint-fix so?

I'm hesitant to put that in the critical path of PRs. What if we don't agree with one of the fixes that it makes? I've never personally used the --fix flag or the lint-fix target, so I don't have first-hand experience, but it always struck me as a convenience wrapper to run if the linter failed to quickly resolve whatever it's complaining about. And then contributors would have a chance to manually review the changes before committing.

the --fix usually fix spaces and go fmt issues. It does not fix all scenarios. if we add the lint check gofmt in https://github.com/operator-framework/operator-sdk/blob/master/.golangci.yml#L2 which I think should have, then we can replace the go fmt ./... here by the lint fix. PS: the go vet is also missing in the lint checks. (govet check)

Makefile

joelanford · 2020-10-19T12:52:12Z

@camilamacedo86 As I mentioned before, my goal in this PR was to simplify the Makefile and decrease the number of make targets. Samples generation has been folded in under the make generate target

The generate target depends on the build target, which compiles the binaries in the build/ directory. The Makefile now also includes the build/ directory as the first item in $PATH, so it is not necessary to globally install operator-sdk to generate the samples.

camilamacedo86

I am ok with we move forward with this one 👍
/lgtm

IHMO: the lint is missing the checks gofmt and govet and then we do not need to it in the sanity. Also, instead of having them in the fix we could call the lint -fix which solve these checks + the spaces issues such as when is missing that at the end of the file.

We can do it a follow up as well.

…nd the image

openshift-ci-robot requested review from bharathi-tenneti and fabianvf October 13, 2020 16:34

camilamacedo86 reviewed Oct 13, 2020

View reviewed changes

internal/testutils/utils.go Show resolved Hide resolved

camilamacedo86 reviewed Oct 13, 2020

View reviewed changes

test/e2e-ansible/e2e_ansible_cluster_test.go Show resolved Hide resolved

bharathi-tenneti removed their request for review October 13, 2020 17:17

camilamacedo86 reviewed Oct 13, 2020

View reviewed changes

camilamacedo86 reviewed Oct 14, 2020

View reviewed changes

.gitignore Show resolved Hide resolved

camilamacedo86 reviewed Oct 14, 2020

View reviewed changes

Makefile Show resolved Hide resolved

camilamacedo86 reviewed Oct 14, 2020

View reviewed changes

tools/scripts/fetch Show resolved Hide resolved

camilamacedo86 approved these changes Oct 14, 2020

View reviewed changes

camilamacedo86 reviewed Oct 14, 2020

View reviewed changes

.golangci.yml Show resolved Hide resolved

estroz reviewed Oct 14, 2020

View reviewed changes

Makefile Show resolved Hide resolved

estroz approved these changes Oct 14, 2020

View reviewed changes

openshift-ci-robot assigned estroz Oct 14, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020

camilamacedo86 mentioned this pull request Oct 14, 2020

*: clean up samples #4035

Merged

2 tasks

estroz reviewed Oct 14, 2020

View reviewed changes

internal/testutils/utils.go Outdated Show resolved Hide resolved

estroz reviewed Oct 14, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020

joelanford force-pushed the makefile-ci-reset branch from b748fdc to 6b37b5e Compare October 15, 2020 15:50

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2020

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020

estroz reviewed Oct 15, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

joelanford force-pushed the makefile-ci-reset branch from 6b37b5e to 8bab1f9 Compare October 15, 2020 19:20

joelanford mentioned this pull request Oct 18, 2020

cleanup: call install in the sample target #4057

Closed

camilamacedo86 requested changes Oct 19, 2020

View reviewed changes

camilamacedo86 reviewed Oct 19, 2020

View reviewed changes

Makefile Show resolved Hide resolved

camilamacedo86 approved these changes Oct 19, 2020

View reviewed changes

joelanford added 7 commits October 19, 2020 10:00

Makefile,*: big cleanup and refactoring of Makefile and scripts

3c649df

hack,test: move image kind loading to tests that actually need kind a…

3839e64

…nd the image

[travis deploy] cleaning up and updating .gitignore

6736845

[travis deploy] use dist directory for release builds

c96b94d

tools/scripts/fetch: honor envtest version, fix variable name

13158c8

addressing PR comments

f804b80

don't trim operator-sdk dir from trimpath

c8b9cea

joelanford force-pushed the makefile-ci-reset branch from 382a998 to c8b9cea Compare October 19, 2020 14:00

joelanford merged commit e311086 into operator-framework:master Oct 19, 2020

joelanford deleted the makefile-ci-reset branch October 19, 2020 17:31

This was referenced Oct 21, 2020

fix incorrect build command in installation doc #4075

Merged

WIP POC To solve flake - fix: ci - install kustomize in the CI #4079

Closed

estroz mentioned this pull request Oct 21, 2020

e2e tests wont run locally #3871

Closed

camilamacedo86 mentioned this pull request Dec 16, 2020

Unable to run the tests locally (Mac) after the changes made on the Makefile #4151

Closed

		Expect(tc.LoadImageToKindClusterWithName("quay.io/operator-framework/scorecard-test:dev")).To(Succeed())
		Expect(tc.LoadImageToKindClusterWithName("quay.io/operator-framework/custom-scorecard-tests:dev")).To(Succeed())

Makefile,*: cleanup and refactoring, support reproducible tests locally #4023

Makefile,*: cleanup and refactoring, support reproducible tests locally #4023

Uh oh!

Conversation

joelanford commented Oct 13, 2020

Uh oh!

joelanford commented Oct 13, 2020

Uh oh!

joelanford commented Oct 13, 2020

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 15, 2020

Uh oh!

Uh oh!

estroz commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joelanford commented Oct 19, 2020

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camilamacedo86 Oct 13, 2020 •

edited

Loading

camilamacedo86 Oct 14, 2020 •

edited

Loading

camilamacedo86 Oct 14, 2020 •

edited

Loading

camilamacedo86 Oct 19, 2020 •

edited

Loading

estroz commented Oct 15, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 Oct 19, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading