⚠️ container friendly commands #4288
Conversation
Signed-off-by: Mateus Oliveira <[email protected]>
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mateusoliveira43 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @mateusoliveira43. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
| @@ -194,7 +194,7 @@ docker-push: ## Push docker image with the manager. | |||
| # To adequately provide solutions that are compatible with multiple platforms, you should consider using this option. | |||
| PLATFORMS ?= linux/arm64,linux/amd64,linux/s390x,linux/ppc64le | |||
| .PHONY: docker-buildx | |||
| docker-buildx: ## Build and push docker image for the manager for cross-platform support | |||
| docker-buildx: ## Build and push container image for the manager for cross-platform support | |||
| # copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile | |||
| sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross | |||
| - $(CONTAINER_TOOL) buildx create --name {{ .ProjectName }}-builder | |||
There was a problem hiding this comment.
TODO change $(CONTAINER_TOOL) here in this command to docker
| @@ -178,12 +178,12 @@ run: manifests generate fmt vet ## Run a controller from your host. | |||
| # If you wish to build the manager image targeting other platforms you can use the --platform flag. | |||
| # (i.e. docker build --platform linux/arm64). However, you must enable docker buildKit for it. | |||
| # More info: https://docs.docker.com/develop/develop-images/build_enhancements/ | |||
| .PHONY: docker-build | |||
| docker-build: ## Build docker image with the manager. | |||
There was a problem hiding this comment.
This change could significantly impact our users, as these targets are likely used in CI pipelines and other automated processes. While I’m open to switching from docker to container term, we need to carefully weigh the pros and cons.
Additionally, we would need to update all relevant documentation, so it’s not as straightforward as it seems. I’m uncertain if the benefits here outweigh the potential downsides.
There was a problem hiding this comment.
yeah, waiting to see if community wants the change before changing the other places
| - docker version 17.03+. | ||
| - kubectl version v1.11.3+. | ||
| - Access to a Kubernetes v1.11.3+ cluster. | ||
| - a container tool (Docker, Podman, Buildah, etc) |
There was a problem hiding this comment.
The issue is that we can't claim full support for all container tools since our testing is limited to Docker. To support additional tools, we would need to test across each of them as well, which would be a considerable burden for us as maintainers. Don't you think?
There was a problem hiding this comment.
So, my though here was where the requirement appears, which is the Getting Started section of the scaffolded README. In this section, we just need to build and push a container image to run the steps described. So should be ok for not test all options here (I tested with Podman, but not with Buildah, for example)
we also have a comment about not testing all options today
But for other parts of the project, we can still say docker is still required, and no need for changing how is tested (for example, if a section about multi-arch release is added to scaffolded README, docker-buildx command would be necessary and then, docker would be necessary requirement for that section)
what you think?
There was a problem hiding this comment.
Yes, we added a comment to clarify that it MAYBE compatible with other tools. However, it’s not tested or officially supported. We avoided renaming the targets to prevent unnecessary disruptions for developers and extensive changes.
This isn't just a matter of updating a few files—it would impact our documentation, CI, and other templates like the README**. I don’t think we should proceed with this right now.**
Perhaps in the future, but not at this point when we need to push changes that require developer effort to ensure compatible with controller-runtime, to fix old bugs and etc. Changing a name or wording just to imply broader tool support which btw is not, isn’t worth the hassle. This might be better suited for a major update, like when we introduce go/v5, for example.
There was a problem hiding this comment.
For example, if we allow the tests run here it will broke all our CI as well.
There was a problem hiding this comment.
agree with waiting for a major update to merge this one
There was a problem hiding this comment.
So, could you please track an issue with the request, and we can create a go/v5 milestone to add this one there?
When we have enough request changes, we can create a new version and do this one.
Also, could you please link this PR there as a reference and close it?
There was a problem hiding this comment.
created #4293
Why the changes were made
Replace
dockerwithcontainerfrom command names that do not necessarily need Docker to be used, but should work with any container tool (Podman, Buildah, etc).How the changes were made
Checked places that mentions
dockerwith the commandgrep -Inr "docker" ./pkg/plugins/golang/v4/scaffolds/internal/templates/and changed them to a container friendly wording.
How to test the changes made
Run new
container-buildandcontainer-pushcommands, and confirm they work asdocker-buildanddocker-pushused to work.