kubernetes-sigs / e2e-framework

A Go framework for end-to-end testing of components running in Kubernetes clusters.
Apache License 2.0
529 stars 103 forks source link

third_party/ko: Add ko as third party tools #415

Closed heylongdacoder closed 5 months ago

heylongdacoder commented 6 months ago

Add Install, BuildLocal and GetLocalImage functions.

What type of PR is this?

/kind feature

What this PR does / why we need it:

Already explained in #343

Which issue(s) this PR fixes:

Partially addresses #343

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., Usage docs, etc.:

N/A
k8s-ci-robot commented 6 months ago

Hi @heylongdacoder. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
heylongdacoder commented 6 months ago

Hi @vladimirvivien, I just realized you were suggesting the BuildLocal function should build and publish to a local Docker. This PR is about ko and a KinD cluster. Since KinD is a popular way to test controllers and operators, I think this PR might be helpful. If so, maybe we can merge this PR first and then create another follow-up PR for the BuildLocal (Docker) and BuildRemote (Docker) functions. WDYT? 😄

vladimirvivien commented 6 months ago

@heylongdacoder this is great. So, does this PR allow local build of images and deploy images into a running Kind cluster?

@harshanarayana cc

cpanato commented 6 months ago

it looks great, just one question why not use ko as lib instead need to have that installed and use via shell?

vladimirvivien commented 6 months ago

+1 on @cpanato question.

heylongdacoder commented 6 months ago

does this PR allow local build of images and deploy images into a running Kind cluster?

@vladimirvivien yes, you are right 😄

why not use ko as lib

@cpanato @vladimirvivien , I never thought about this because other tools were installed and use via shell. But if we want to use ko as lib, do we need to ensure ko maintainers agree to maintain those functions signature?

harshanarayana commented 6 months ago

+1 from me too. I like the idea of integrating this as a lib instead of the CLI.

I have been using ko as a lib for my work related item for a while in dev mode. They even have a doc around it. https://ko.build/advanced/go-packages/

With this in place, I suppose there is going to be some guarantee on backward compatibility.

That being said, Only concern I have is all the dependencies that will come along with ko into e2e-framework (framework has very little dependency outside of the k/* repo right now). Direct/transient. Do we see it leading to some issue in the future ? with the bin approach, we don't have to worry about that complication

harshanarayana commented 6 months ago

So, does this PR allow local build of images and deploy images into a running Kind cluster?

We should be able to chain the build call to the kind providers load image to get this working. Right ? @vladimirvivien

tsl409 commented 6 months ago

+1 definitely nice to have!

cpanato commented 6 months ago

I think, for now, we can keep it as is. We have other tools that follow this approach. and as a follow up, we can migrate to use as a lib

k8s-ci-robot commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, heylongdacoder

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/e2e-framework/blob/main/OWNERS)~~ [cpanato] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
vladimirvivien commented 5 months ago

Thanks @heylongdacoder this is great.

All good points raised here. Brining ko Go dependency could be overkill and since Kind uses OS commands, I think this is Ok as well.

As @harshanarayana mentioned, I think it a user should be able to load the docker image in Kind. Maybe an example to show the whole cycle of build and load would be useful.

heylongdacoder commented 5 months ago

Maybe an example to show the whole cycle of build and load would be useful.

https://github.com/kubernetes-sigs/e2e-framework/pull/415/files#diff-5e974e6375efb2beaa1644b28e57ba8e5c006b9dab1b47620b4edc24cd50d042R40, it is actually here. The KinD cluster is created in main_test.go. Then, the ko BuildLocal function is used to build the testdata/example_goapp and load it into the KinD cluster using its name. Do you all think this is good enough? 😄

cpanato commented 5 months ago

/unhold

heylongdacoder commented 5 months ago

thanks for all the review!