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

Use k8s-test-infra devel image #86

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

ArangoGutierrez
Copy link
Collaborator

No description provided.

Makefile Outdated Show resolved Hide resolved
@ArangoGutierrez
Copy link
Collaborator Author

@elezar PR is now rebased

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@ArangoGutierrez
Copy link
Collaborator Author

last week @elezar and I had a quick ad-hock conversation on whether it makes sense to continue to push for a centralized image (the k8s-test-infra) image, or we should roll back the per-repo devel image, just with some rules so it looks similar across all our repos. So far the centralized image has added no value, and perhaps has added complications like the one leading to this PR.

If @elezar / @klueska think we should roll back, I'll have it as a TODO and prepare a per-repo solution.

@ArangoGutierrez ArangoGutierrez requested a review from klueska April 15, 2024 15:41
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@klueska
Copy link
Collaborator

klueska commented Apr 16, 2024

last week @elezar and I had a quick ad-hock conversation on whether it makes sense to continue to push for a centralized image (the k8s-test-infra) image, or we should roll back the per-repo devel image, just with some rules so it looks similar across all our repos. So far the centralized image has added no value, and perhaps has added complications like the one leading to this PR.

If @elezar / @klueska think we should roll back, I'll have it as a TODO and prepare a per-repo solution.

I think a centralized one is fine as long as it's configurable for each repo.

@elezar
Copy link
Member

elezar commented Apr 16, 2024

I think a centralized one is fine as long as it's configurable for each repo.

My point was that our repos represent a disjoint set of components with different requirements and lifecycles. Trying to consolidate tooling in a central location may be effort that doesn't serve a direct purpose at present. Adding per-repo customization removes the primary benefit of this which is image reuse since a local image needs to be built for non-standard requirements. Furthermore, a user may not be aware of the fact that they NEED to build a custom image for local development since this is not explicitly encoded in the makefile.

Looking at the current devel image's Dockerfile the use of moq, golangci-lint, and setting the git config options on the /work directory are the only three things that are (somewhat) universally applicable. The other tooling was added for projects such as the k8s-dra-driver (although it may be used in some other projects). When considering codebases such as go-nvml the tooling becomes more specific and it may not make sense to centrally manage these.

@klueska
Copy link
Collaborator

klueska commented Apr 16, 2024

I don't feel strongly either way.

On one hand, I like the per-repo images that we had originally because I could just update them as necessary for the particular project at hand, without worrying that I might "break" some other project (or become out of sync with some centralized image). This image is only meant to be used locally for build / devel purposes, so it makes sense to also define / build it locally instead of pulling it from some centralized location.

On the other hand, I can see how it can quickly become hard to keep the set of local Dockerfiles we have consistent across repos (for the parts of them that can/should be kept consistent).

Does it then make sense to have a "base" image hosted in k8s-test-infra that only contains the base set of tooling we know all devel images should have and then have each repo define a local Dockerfile that uses this as a base?

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@ArangoGutierrez
Copy link
Collaborator Author

Rebased and addressed comments

Copy link
Collaborator

@klueska klueska left a comment

Choose a reason for hiding this comment

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

Let's merge this for now. We should continue to discuss whether we want a local vs. remote image, but the default build is broken at the moment and we need something that will fix it.

@klueska klueska merged commit caab415 into NVIDIA:main Apr 25, 2024
5 checks passed
@ArangoGutierrez ArangoGutierrez deleted the remotedockerfile branch April 25, 2024 11:59
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.

3 participants