*: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' #4226

wking · 2020-11-11T20:16:13Z

Because folks may have docker aliased to podman, and Podman prefers options before positional arguments:

$ podman build --help | grep CONTEXT-DIRECTORY
   podman build [command options] CONTEXT-DIRECTORY | URL

Generated with:

$ sed -i 's/docker build . -t ${IMG}/docker build -t ${IMG} ./' $(git grep -l 'docker.*build \. ')

Because folks may have 'docker' aliased to 'podman', and Podman prefers options before positional arguments: $ podman build --help | grep CONTEXT-DIRECTORY podman build [command options] CONTEXT-DIRECTORY | URL Generated with: $ sed -i 's/docker build . -t ${IMG}/docker build -t ${IMG} ./' $(git grep -l 'docker.*build \. ')

joelanford · 2020-11-11T20:22:28Z

Seems reasonable to me. If applicable, we should also make this change upstream in the Kubebuilder plugins so we can pull the same change in for Go projects.

camilamacedo86

Hi @wking,

Thank you for your suggestion. However, we have 3 types of operators that are built with the tool and in order to keep its maintainability and give a better user experience, I do not think that we should change the common targets only for Ansible.

So, to move forward with this change IHMO we need to first push it to upstream/kubebuiler. See: https://github.com/kubernetes-sigs/kubebuilder. Would you like to push a PR against upstream?

Then, when a new version of the upstream/kb be bumped here we can also apply these changes for the other types as well.

However, before you push your suggestion to upstream could you please clarifies better what use case it will address? Sorry, but I did not get what is not possible to do currently with docker build . -t ${IMG} that will be solved with the replace docker build -t ${IMG} .

/hold

Generated with: $ git checkout -b sdk-0.19 origin/master $ git merge --allow-unrelated-histories 0.19-scaffolding $ git checkout --theirs .gitignore # drop lots of user-editor cruft, see "Which file to place a pattern in..." in https://git-scm.com/docs/gitignore#_description $ emacs api/v1beta1/cincinnati_types.go # keep some of the outgoing pkg/apis/cincinnati/v1beta1/cincinnati_types.go content $ emacs controllers # keep some of the outgoing pkg/controller content $ emacs main.go # incorperate some of the outgoing cmd/manager/main.go content $ emacs Dockerfile # keep most of the pre-0.19 content. We will still vendor deps, and don't want to have GOARCH, etc., opinions $ emacs Makefile # keep most of the 0.19 content, with a few pre-0.19 rules for openshift/release compatability $ emacs README.md # bump SDK references to 0.19 $ make manifests $ git add .gitignore config Dockerfile main.go Makefile README.md $ git rm -rf cmd/manager pkg/apis pkg/controller $ git checkout HEAD -- go.sum # we'll auto-generate this in a future commit $ emacs go.mod # basically a union of the two sets. Drop the hopefully-obsolete 'replace' block. $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.15.2 linux/amd64 $ kustomize version {Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64} $ controller-gen --version Version: v0.3.0 'oc' has a kustomize subcommand, but even in the master oc branch, that is currently v2 [1], while 'kustomize edit ...' needs v3 to avoid [2]: cd config/manager && /cli/oc kustomize edit set image controller=controller:latest Error: specify one path to a kustomization directory The 'docker build . -t ${IMG}' -> 'docker build -t ${IMG} .' change is for Podman compatibility [3]. Commenting out the 'test' prerequisite allows us to run 'make docker-build' without having functests try to run tests in whatever cluster you may be pointed at. [1]: https://github.com/openshift/oc/blob/1bd1f5ff4aa98d6e4184c2948928b6111fa99191/vendor/modules.txt#L1293 [2]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/76/pull-ci-openshift-cincinnati-operator-master-operator-e2e/1326532618765733888 [3]: operator-framework/operator-sdk#4226

wking · 2020-11-11T21:55:35Z

Podman folks talking about this in containers/podman#2811.

I do not think that we should change the common targets only for Ansible.

In this PR, I'm changing everything that matched my sed. Did I miss an entry? I will file an upstream PR.

wking · 2020-11-11T22:00:44Z

Upstreamed as kubernetes-sigs/kubebuilder#1817

Generated with: $ git checkout -b sdk-0.19 origin/master $ git merge --allow-unrelated-histories 0.19-scaffolding $ git checkout --theirs .gitignore # drop lots of user-editor cruft, see "Which file to place a pattern in..." in https://git-scm.com/docs/gitignore#_description $ emacs api/v1beta1/cincinnati_types.go # keep some of the outgoing pkg/apis/cincinnati/v1beta1/cincinnati_types.go content $ emacs controllers # keep some of the outgoing pkg/controller content $ emacs main.go # incorperate some of the outgoing cmd/manager/main.go content $ emacs Dockerfile # keep most of the pre-0.19 content. We will still vendor deps, and don't want to have GOARCH, etc., opinions $ emacs Makefile # keep most of the 0.19 content, with a few pre-0.19 rules for openshift/release compatability $ emacs README.md # bump SDK references to 0.19 $ make manifests $ git add .gitignore config Dockerfile main.go Makefile README.md $ git rm -rf cmd/manager pkg/apis pkg/controller $ git checkout HEAD -- go.sum # we'll auto-generate this in a future commit $ emacs go.mod # basically a union of the two sets. Drop the hopefully-obsolete 'replace' block. $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.15.2 linux/amd64 $ kustomize version {Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64} $ controller-gen --version Version: v0.3.0 'oc' has a kustomize subcommand, but even in the master oc branch, that is currently v2 [1], while 'kustomize edit ...' needs v3 to avoid [2]: cd config/manager && /cli/oc kustomize edit set image controller=controller:latest Error: specify one path to a kustomization directory The 'docker build . -t ${IMG}' -> 'docker build -t ${IMG} .' change is for Podman compatibility [3]. Commenting out the 'test' prerequisite allows us to run 'make docker-build' without having functests try to run tests in whatever cluster you may be pointed at. [1]: https://github.com/openshift/oc/blob/1bd1f5ff4aa98d6e4184c2948928b6111fa99191/vendor/modules.txt#L1293 [2]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/76/pull-ci-openshift-cincinnati-operator-master-operator-e2e/1326532618765733888 [3]: operator-framework/operator-sdk#4226

Generated with: $ git checkout -b sdk-0.19 origin/master $ git merge --allow-unrelated-histories 0.19-scaffolding $ git checkout --theirs .gitignore # drop lots of user-editor cruft, see "Which file to place a pattern in..." in https://git-scm.com/docs/gitignore#_description $ emacs api/v1beta1/cincinnati_types.go # keep some of the outgoing pkg/apis/cincinnati/v1beta1/cincinnati_types.go content $ emacs controllers # keep some of the outgoing pkg/controller content $ emacs main.go # incorperate some of the outgoing cmd/manager/main.go content $ emacs Dockerfile # keep most of the pre-0.19 content. We will still vendor deps, and don't want to have GOARCH, etc., opinions $ emacs Makefile # keep most of the 0.19 content, with a few pre-0.19 rules for openshift/release compatability $ emacs README.md # bump SDK references to 0.19 $ make manifests $ git add .gitignore config Dockerfile main.go Makefile README.md $ git rm -rf cmd/manager pkg/apis pkg/controller $ git checkout HEAD -- go.sum # we'll auto-generate this in a future commit $ emacs go.mod # basically a union of the two sets. Drop the hopefully-obsolete 'replace' block. $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.15.2 linux/amd64 $ kustomize version {Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64} $ controller-gen --version Version: v0.3.0 'oc' has a kustomize subcommand, but even in the master oc branch, that is currently v2 [1], while 'kustomize edit ...' needs v3 to avoid [2]: cd config/manager && /cli/oc kustomize edit set image controller=controller:latest Error: specify one path to a kustomization directory The 'docker build . -t ${IMG}' -> 'docker build -t ${IMG} .' change is for Podman compatibility [3]. Commenting out the 'test' prerequisite allows us to run 'make docker-build' without having functests try to run tests in whatever cluster you may be pointed at. The increased memory limits are based on peak measurements of ~11m CPU and ~94MiB in a 4.6.4 amd64 cluster with a single Cincinnati object. [1]: https://github.com/openshift/oc/blob/1bd1f5ff4aa98d6e4184c2948928b6111fa99191/vendor/modules.txt#L1293 [2]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/76/pull-ci-openshift-cincinnati-operator-master-operator-e2e/1326532618765733888 [3]: operator-framework/operator-sdk#4226

Generated with: $ git checkout -b sdk-0.19 origin/master $ git merge --allow-unrelated-histories 0.19-scaffolding $ git checkout --theirs .gitignore # drop lots of user-editor cruft, see "Which file to place a pattern in..." in https://git-scm.com/docs/gitignore#_description $ emacs api/v1beta1/cincinnati_types.go # keep some of the outgoing pkg/apis/cincinnati/v1beta1/cincinnati_types.go content $ emacs controllers # keep some of the outgoing pkg/controller content $ emacs main.go # incorperate some of the outgoing cmd/manager/main.go content $ emacs Dockerfile # keep most of the pre-0.19 content. We will still vendor deps, and don't want to have GOARCH, etc., opinions $ emacs Makefile # keep most of the 0.19 content, with a few pre-0.19 rules for openshift/release compatability $ emacs README.md # bump SDK references to 0.19 $ make manifests $ git add .gitignore config Dockerfile main.go Makefile README.md $ git rm -rf cmd/manager pkg/apis pkg/controller $ git checkout HEAD -- go.sum # we'll auto-generate this in a future commit $ emacs go.mod # basically a union of the two sets. Drop the hopefully-obsolete 'replace' block. $ make bundle $ emacs config/manifests/bases/cincinnati-operator.clusterserviceversion.yaml # keep some of the outgoing deploy/olm-catalog content $ make bundle $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.15.2 linux/amd64 $ kustomize version {Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64} $ controller-gen --version Version: v0.3.0 'oc' has a kustomize subcommand, but even in the master oc branch, that is currently v2 [1], while 'kustomize edit ...' needs v3 to avoid [2]: cd config/manager && /cli/oc kustomize edit set image controller=controller:latest Error: specify one path to a kustomization directory The 'docker build . -t ${IMG}' -> 'docker build -t ${IMG} .' change is for Podman compatibility [3]. Commenting out the 'test' prerequisite allows us to run 'make docker-build' without having functests try to run tests in whatever cluster you may be pointed at. The increased memory limits are based on peak measurements of ~11m CPU and ~94MiB in a 4.6.4 amd64 cluster with a single Cincinnati object. [1]: https://github.com/openshift/oc/blob/1bd1f5ff4aa98d6e4184c2948928b6111fa99191/vendor/modules.txt#L1293 [2]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/76/pull-ci-openshift-cincinnati-operator-master-operator-e2e/1326532618765733888 [3]: operator-framework/operator-sdk#4226

Generated by creating a fresh scaffold: $ git checkout --orphan 0.19-scaffolding $ git rm -rf . $ operator-sdk-v0.19.3 init --domain openshift.io --repo github.com/openshift/cincinnati-operator $ operator-sdk-v0.19.3 create api --group cincinnati --version v1beta1 --kind Cincinnati --resource --controller $ git add -A .gitignore * The following paths are ignored by one of your .gitignore files: bin hint: Use -f if you really want to add them. hint: Turn this message off by running hint: "git config advice.addIgnoredFile false" And then, back on master: $ git checkout -b sdk-0.19 origin/master $ git merge --allow-unrelated-histories 0.19-scaffolding $ git checkout --theirs .gitignore # drop lots of user-editor cruft, see "Which file to place a pattern in..." in https://git-scm.com/docs/gitignore#_description $ emacs api/v1beta1/cincinnati_types.go # keep some of the outgoing pkg/apis/cincinnati/v1beta1/cincinnati_types.go content $ emacs controllers # keep some of the outgoing pkg/controller content $ emacs main.go # incorperate some of the outgoing cmd/manager/main.go content $ emacs Dockerfile # keep most of the pre-0.19 content. We will still vendor deps, and don't want to have GOARCH, etc., opinions $ emacs Makefile # keep most of the 0.19 content, with a few pre-0.19 rules for openshift/release compatability $ emacs README.md # bump SDK references to 0.19 $ make manifests $ git add .gitignore config Dockerfile main.go Makefile README.md $ git rm -rf cmd/manager pkg/apis pkg/controller $ git checkout HEAD -- go.sum # we'll auto-generate this in a future commit $ emacs go.mod # basically a union of the two sets. Drop the hopefully-obsolete 'replace' block. $ make bundle $ emacs config/manifests/bases/cincinnati-operator.clusterserviceversion.yaml # keep some of the outgoing deploy/olm-catalog content $ make bundle $ go mod tidy $ go mod vendor $ git add -A go.* vendor $ git commit And then washing the merge out of the history: $ git branch temporary-merge $ git reset --hard HEAD^ $ git rm -rf . $ git checkout temporary-merge -- . $ git commit $ git branch -D temporary-merge using: $ go version go version go1.15.2 linux/amd64 $ kustomize version {Version:kustomize/v3.8.6 GitCommit:c1747439cd8bc956028ad483cdb30d9273c18b24 BuildDate:2020-10-29T23:07:50Z GoOs:linux GoArch:amd64} $ controller-gen --version Version: v0.3.0 'oc' has a kustomize subcommand, but even in the master oc branch, that is currently v2 [1], while 'kustomize edit ...' needs v3 to avoid [2]: cd config/manager && /cli/oc kustomize edit set image controller=controller:latest Error: specify one path to a kustomization directory The 'docker build . -t ${IMG}' -> 'docker build -t ${IMG} .' change is for Podman compatibility [3]. Commenting out the 'test' prerequisite allows us to run 'make docker-build' without having functests try to run tests in whatever cluster you may be pointed at. The increased memory limits are based on peak measurements of ~11m CPU and ~94MiB in a 4.6.4 amd64 cluster with a single Cincinnati object. [1]: https://github.com/openshift/oc/blob/1bd1f5ff4aa98d6e4184c2948928b6111fa99191/vendor/modules.txt#L1293 [2]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cincinnati-operator/76/pull-ci-openshift-cincinnati-operator-master-operator-e2e/1326532618765733888 [3]: operator-framework/operator-sdk#4226

wking · 2020-11-18T05:05:18Z

Upstream PR landed. Do we pull the hold here, or is there another way to ingest the upstream change?

jmrodri

/lgtm

camilamacedo86

IMO we should not merge this one.
This change was made for v3/go-alpha (kubebuilder/upstream see: kubernetes-sigs/kubebuilder#1817 ). In this way, when SDK starts to support Golang v3/alpha then, we need to create the v2+ plugins for Ansible/Helm where the changes made for v3 would be applied to them as well. At this moment this change will be applied for these types.

joelanford · 2020-11-23T16:11:08Z

Despite the upstream conversation to the contrary, I don't think I would consider this a change that needs to be held until a new version of the ansible and helm plugins are released. IMO, we should have pushed harder for this to be backported to the Go v2 plugin upstream. I also don't think the upstream decision binds our downstream decision.

There are always going to be differences between the different language plugins, and I think we can make our own decisions about what features and fixes to include in the helm and ansible plugins even if upstream is very conservative about what constitutes a breaking change in the Go plugin's scaffolded files.

I think I'm a 👍 to getting this merged.

@joe

I would prefer address this improvement at the same time that we have this change in the Golang for a few reasons such as it make easier we keep the project maintained since we know how they work and to keep all work as closer as possible. However, since shows that @joe is ok with we not provide the same user experience for the users and with it then I am dimissing my review here.

camilamacedo86 · 2020-11-23T16:17:43Z

Hi @wking,

See that for it get merged it needs to pass in the CI. It is falling in the make test-sanity. to fix that you need to run make generate.

camilamacedo86 · 2020-11-23T16:18:08Z

internal/plugins/ansible/v1/scaffolds/internal/templates/makefile.go

@@ -93,7 +93,7 @@ undeploy: kustomize



Also, it is missing the fragment. Changelog.

jmrodri · 2021-02-03T00:45:37Z

Closing this in favor of PR #4466 to avoid the constant back and forth.

openshift-ci-robot requested review from camilamacedo86 and fabianvf November 11, 2020 20:16

camilamacedo86 reviewed Nov 11, 2020

View reviewed changes

camilamacedo86 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2020

joelanford added language/ansible Issue is related to an Ansible operator project language/helm Issue is related to a Helm operator project labels Nov 12, 2020

joelanford mentioned this pull request Nov 12, 2020

Apply updates from operator-sdk helm-operator operator-framework/helm-operator-plugins#63

Open

10 tasks

jmrodri approved these changes Nov 23, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Nov 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020

camilamacedo86 previously requested changes Nov 23, 2020

View reviewed changes

camilamacedo86 reviewed Nov 23, 2020

View reviewed changes

jmrodri mentioned this pull request Feb 3, 2021

*: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' #4466

Merged

2 tasks

jmrodri closed this Feb 3, 2021

wking deleted the podman-build-compat branch February 17, 2021 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' #4226

*: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' #4226

wking commented Nov 11, 2020

joelanford commented Nov 11, 2020

camilamacedo86 left a comment •

edited

Loading

wking commented Nov 11, 2020

wking commented Nov 11, 2020

wking commented Nov 18, 2020

jmrodri left a comment

camilamacedo86 left a comment •

edited

Loading

joelanford commented Nov 23, 2020

camilamacedo86 commented Nov 23, 2020

camilamacedo86 Nov 23, 2020

jmrodri commented Feb 3, 2021

*: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' #4226

*: Replace 'docker build . -t ${IMG}' with '-t ${IMG} .' #4226

Conversation

wking commented Nov 11, 2020

joelanford commented Nov 11, 2020

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

wking commented Nov 11, 2020

wking commented Nov 11, 2020

wking commented Nov 18, 2020

jmrodri left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

joelanford commented Nov 23, 2020

camilamacedo86 commented Nov 23, 2020

camilamacedo86 Nov 23, 2020

Choose a reason for hiding this comment

jmrodri commented Feb 3, 2021

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 left a comment •

edited

Loading