fixes to have test-*-local Makefile targets work #2654
Conversation
lsm5
changed the title
|
Moving the TMT-independent stuff from #2640 here for easier review. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Just an immediate skim, not nearly a review,
|
A friendly reminder that this PR had no activity for 30 days. |
|
Tests failed. @containers/packit-build please check. |
lsm5
force-pushed
the
ci-agnostic
branch
3 times, most recently
from
a7f91f2 to
e8a0b12
Compare
lsm5
force-pushed
the
ci-agnostic
branch
6 times, most recently
from
23980b7 to
e44f514
Compare
|
I've replaced I'll make a followup PR for some other packit related changes to avoid overwriting the downstream changes and ensure the git refs are updated on every release. |
|
Btw, we will also be able to get rid of the |
This is required for fetching golangci-lint if it doesn't exist. Signed-off-by: Lokesh Mandvekar <[email protected]>
Use inline script and get rid of hack/test-system.sh Signed-off-by: Lokesh Mandvekar <[email protected]>
Get rid of hack/test-integration.sh Signed-off-by: Lokesh Mandvekar <[email protected]>
Reuses Makefile logic and also prints SKOPEO_BINARY value Signed-off-by: Lokesh Mandvekar <[email protected]>
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
LGTM overall, feel free to merge — just noting some items that might be worth another look.
| test-system-local: $(if $(SKOPEO_BINARY),,bin/skopeo) | ||
| hack/warn-destructive-tests.sh | ||
| hack/test-system.sh SKOPEO_LDFLAGS="$(SKOPEO_LDFLAGS)" BUILDTAGS="$(BUILDTAGS)" | ||
| @echo "Testing with $(or $(SKOPEO_BINARY),$(eval SKOPEO_BINARY := "bin/skopeo")$(SKOPEO_BINARY)) ..." |
There was a problem hiding this comment.
AFAICS this does not export SKOPEO_BINARY as an environment variable, so the eval only affects the printed path here. Was that intentional?
(In practice it ultimately works the same, systemtest/helpers.bash defaults to essentially $(pwd)/bin/skopeo anyway.)
There was a problem hiding this comment.
Yes, this was only meant to echo SKOPEO_BINARY if it's set or just use the default bin/skopeo so we know which binary is actually being tested. We're setting SKOPEO_BINARY in the TMT tests.
| # SKOPEO_CONTAINER_TESTS (for acceptability to do destructive modification) and !CI | ||
| # (for necessity to adjust for in-container operation) | ||
| if ((SKOPEO_CONTAINER_TESTS)) && [[ "$CI" != true ]]; then | ||
| if [[ -r /etc/containers/storage.conf ]]; then |
There was a problem hiding this comment.
Non-blocking: I don’t know about anyone that uses this — so if we are giving up on the “container outside of CI” setup, perhaps we should also explicitly remove the test-system target (and all of check as well??).
Or is this being removed because you have determined it’s no longer necessary?
There was a problem hiding this comment.
True, make test-system will fail without this. So, I'd be in favour of getting rid of this and test-system, and maybe replace the targets in check with -local .
I'd also be in favour of doing the same for test-integration.
That saves us the burden of worrying about skopeo_ci_dev image maintenance too.
Thanks, I'll need an approving review to go ahead with the merge. It's currently blocked for me. |
3 participants
Please see individual commits.