Skip to content
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

fix failed tests are depends on external container images #5328

Conversation

koba1t
Copy link
Member

@koba1t koba1t commented Sep 13, 2023

fix #5319

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 13, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch from 508ba30 to 774c1e2 Compare September 13, 2023 18:13
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch 2 times, most recently from 5408838 to ac848b6 Compare September 20, 2023 21:01
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch 3 times, most recently from d8f7fc0 to 1124aa9 Compare September 21, 2023 13:18
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 27, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch from adae377 to fabd61f Compare September 27, 2023 17:50
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 27, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch 6 times, most recently from 925f8c0 to 7869890 Compare September 28, 2023 06:19
@koba1t koba1t marked this pull request as ready for review September 28, 2023 06:39
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch from 7869890 to 69a2a48 Compare October 5, 2023 11:52
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2023
@@ -0,0 +1,34 @@
module sigs.k8s.io/kustomize/cmd/config/internal/commands/e2e/e2econtainerenvgenerator

go 1.20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koba1t Could you elaborate on why we need a separate go mod file and repo for e2e testing? I'm sorry if the question sounds trivial, but I'm still trying to catch up. The issue states:

"I believe this was caused by attempting to run a container image that was built external to this repo and does not support the arm64 architecture."

but I'm just wondering if we need to init a separate repo for just for e2e. And if its just for building the container image, why do we need to do it specifically inside this directory?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @varshaprasad96
Thanks for your comment.

but I'm just wondering if we need to init a separate repo for just for e2e. And if its just for building the container image

I think we need any container images when testing the KRM container function. And, I think we need separate modules because we need to build small images in tests.

why do we need to do it specifically inside this directory?

I'm just putting the same directory on the source of previous e2e tests container image.

If you need any ideas to improve these tests, Could you tell me that?
I want to know your advice and improve this code!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch from 69a2a48 to 9da0b3d Compare October 21, 2023 06:51
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 21, 2023
@koba1t koba1t force-pushed the fix/failed_tests_are_depends_on_external_container_images branch from 9da0b3d to af7c088 Compare November 1, 2023 17:37
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koba1t apologies for missing the ping. For the tests, one option is to init the project in-memory instead on disk, as that could possibly help us to reduce the number of files in the project just for testing purposes. Plus the scaffolded projects are not for users to refer and do not contain much of code making it easier to have templates for them. One option is with mapFs (https://pkg.go.dev/testing/fstest#MapFS). Using this is easier because it let's us map the files and the contents. We could also use TestContainers (https://golang.testcontainers.org/features/build_from_dockerfile/#build-from-dockerfile) to build and use lightweight docker images for test.

I would not like to hold this PR based on the above suggestions though. We can merge this, and create an issue to investigate and follow up with a clean up, if that would make the process easier.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: koba1t, varshaprasad96

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2023
@k8s-ci-robot k8s-ci-robot merged commit f87942e into kubernetes-sigs:master Nov 1, 2023
9 checks passed
@koba1t koba1t deleted the fix/failed_tests_are_depends_on_external_container_images branch November 1, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

A few tests are failed on the arm64 architecture
3 participants