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

✨Enhancements for running E2E tests locally #981

Merged
merged 7 commits into from
Sep 2, 2021

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Sep 1, 2021

What this PR does / why we need it:
This PR allows me to run the E2E tests in my environment against my local cloud. This requires enhancements in the following areas:

  • Support running against a cloud which uses SSL
  • Support clouds.yaml which specifies user_domain_name
  • Support running against a cloud with multiple external networks
  • Support using podman locally

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 1, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 1, 2021
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 1, 2021

/cc @sbueringer

Add support for clouds.yaml containing:
* cacert
* user_domain_name or project_domain_name
According to the comment, this step pre-populates the cache. However,
the actual build step also populates the cache, so any additional work
done by this step is redundant.
This allows the e2e tests to run against a cloud with multiple external
networks.
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 2, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2021
@jichenjc
Copy link
Contributor

jichenjc commented Sep 2, 2021

/lgtm
/approve

thanks for the enhancement ~

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, mdbooth

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2021
@k8s-ci-robot k8s-ci-robot merged commit bff2282 into kubernetes-sigs:master Sep 2, 2021
@@ -34,11 +34,6 @@ RUN --mount=type=cache,target=/go/pkg/mod \
# Copy the sources
COPY ./ ./

Copy link
Member

@sbueringer sbueringer Sep 2, 2021

Choose a reason for hiding this comment

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

@mdbooth Out of curiosity. Why is the caching removed? I would have assumed that podman is using the Containerfile instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, podman doesn't use this file. I was looking at Dockerfile because I originally trying to create a combined file, but I couldn't make the caching work.

I removed the additional go build step because afaict it's redundant even for docker. If you notice we build again with build arguments below and the same cache. This step is likely caching artifacts which we don't use. The 'real' build will create artifacts for everything we actually use, and nothing else. This step can only result in additional work and doesn't improve cache effectiveness.

...unless I've misunderstood some semantics of buildkit? In which case we can revert this.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're probably right

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants