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

Test: Docker image for tests #5193

Closed
wants to merge 40 commits into from
Closed

Conversation

aman0408
Copy link
Contributor

Fixes #4545

Description

How was this change tested?

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aman0408 aman0408 requested a review from a team as a code owner November 30, 2023 01:13
Copy link

netlify bot commented Nov 30, 2023

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 9d8e836
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/65738ab4e5c8bd00083830b5

test/Dockerfile Outdated
@@ -59,4 +60,7 @@ RUN go version
# Cache modules to speed up test execution
RUN git clone https://github.com/aws/karpenter.git /karpenter
WORKDIR /karpenter
RUN GOPROXY=direct go mod tidy

RUN K8S_VERSION="1.28.x" make toolchain
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to run the full make toolchain? IIRC, we should be fine just defining a subset of dependencies in this Dockerfile and then using those directly.

@@ -3,6 +3,7 @@ set -euo pipefail

K8S_VERSION="${K8S_VERSION:="1.28.x"}"
KUBEBUILDER_ASSETS="/usr/local/kubebuilder/bin"
USER=$(whoami)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the need for adding this to the script?

@jonathan-innis
Copy link
Contributor

Discussed offline a bit. Given the state of the current code and the value prop of adding the Dockerfile, I'm more inclined to forego this change in favor of just installing some of the tooling at runtime. If we start feeling like the overhead of running all of the installs at the beginning of the E2E is too onerous, then I could see us adding this, but I'm not sure it's worth the maintenance burden right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E Tests should use a Docker Image as opposed to installing tooling at runtime
2 participants