test/test-framework: add go.mod, go.sum and build/Dockerfile #2202
Conversation
estroz
requested review from
jmrodri,
joelanford,
jmccormick2001,
hasbro17 and
camilamacedo86
openshift-ci-robot
added
the
size/XL
|
|
||
| # Remove files that are required and created just to exec the commands. | ||
| trap_add 'rm -rf $DOCKERFILE go.mod go.sum' EXIT | ||
|
|
There was a problem hiding this comment.
Does go.sum need to be checked in? If so, maybe we should run go mod tidy in this script to make it easier to keep up-to-date?
There was a problem hiding this comment.
tidy is already run in sanity-check.sh
There was a problem hiding this comment.
I'm not sure that helps us. Does go mod tidy run recursively against all other modules (e.g. test/test-framework) in the repo? I don't think it does.
If we include it here, a developer can run make gen-test-framework after making changes in the test framework to pick up all of the necessary changes. go.mod and go.sum will get updated along with the other generated files.
There was a problem hiding this comment.
Ah I see what you mean. It does not. I will add it here.
test/test-framework/go.mod
Outdated
|
|
||
| require ( | ||
| github.com/go-openapi/spec v0.19.0 | ||
| github.com/operator-framework/operator-sdk v0.12.1-0.20191112211508-82fc57de5e5b |
There was a problem hiding this comment.
Does this become another file that needs to be checked and updated during releases?
I'm wondering if we can incorporate operator-sdk print-deps in the gen-test-framework script to keep this up-to-date?
There was a problem hiding this comment.
It was the reason that I thought would be easier not to keep them committed.
There was a problem hiding this comment.
The replace line at the bottom should take care of that issue.
There was a problem hiding this comment.
I don't think it quite takes care of it. If I understand Go modules correctly, I think this would result in the compiled test-framework using all the resolved require dependencies of github.com/operator-framework/operator-sdk v0.12.1-0.20191112211508-82fc57de5e5b and then substituting just operator-sdk for the one listed in the replace.
There was a problem hiding this comment.
On further investigation it looks like you are correct. This shouldn't be an issue if we do
$ cd test/test-framework
$ operator-sdk print-deps > go.mod
$ echo -e "\nreplace github.com/operator-framework/operator-sdk => ../../" >> go.mod
$ go build ./...
$ go mod tidyin gen-test-framework, as well as update the require version. This will avoid passing incompatible transitive dependencies to test-framework and make sure that pinned dependencies are up-to-date. One somewhat annoying issue that crops up here is that resolving branch master will cause an inconsistent require for the SDK whenever master has been updated. We can get around this with some go mod edit logic.
@camilamacedo86 I understand where you're coming from. Making the test-framework a module adds complexity to our sanity check and release process, while saving some unit tests from writing a mock go.mod. Therefore making test-framework a module might not be the right move.
There was a problem hiding this comment.
I think for the propose to use the testdata as a project mock and ensure that it is updated has no need to do it. If we would like to facilitate the update of the testadata we can create a make target that will do as it is done in the check. Wdyt?
hack/generate/gen-test-framework.sh: remove workarounds for lack of go.mod, go.sum, build/Dockerfile internal/scaffold/crd_test.go: modularizing test/test-framework broke the Go project test because submodules are not packages of a module, which caused CRD generator errors. Changing wd to test/test-framework fixed the issue
estroz
force-pushed
the
test-framework-add-modules
branch
from
cda7cc8
to
c467813
Compare
estroz
force-pushed
the
test-framework-add-modules
branch
from
77153b8
to
2c31fac
Compare
openshift-ci-robot
added
size/M
estroz
force-pushed
the
test-framework-add-modules
branch
2 times, most recently
from
e58e863
to
0738ae9
Compare
openshift-ci-robot
added
size/XL
estroz
force-pushed
the
test-framework-add-modules
branch
from
0738ae9
to
f0bc771
Compare
| trap_add 'rm -rf $DOCKERFILE go.mod go.sum' EXIT | ||
| # Ensure test-framework is up-to-date with current Go project dependencies. | ||
| # NOTE: the SDK dependency version still needs updating on a new release. | ||
| sdk_version="$(go list -m -f '{{.Version}}' github.com/operator-framework/operator-sdk)" |
There was a problem hiding this comment.
TIL about this command! If there's equivalent go code, maybe in a future PR, we should consider outputting this version in the go.mod scaffold directly. Though maybe it's moot with the kubebuilder transition.
hack/generate/gen-test-framework.sh
Outdated
| trap_add 'rm -f pkg/apis/cache/v1alpha1/zz_generated.openapi.go' EXIT |
There was a problem hiding this comment.
In @camilamacedo86's previous PR, we intentionally left off the -f flag to rm so that if we accidentally forget about removing these lines when we get rid of the openapi.go code generator, this will fail, and we will be reminded to remove this.
Description of the change:
Motivation for the change: test/test-framework contains a working operator that we use for testing. It should be as close to a standalone operator built with the SDK as possible.