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

make/targets: version binaries in _output #48

Merged

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jul 12, 2021

No description provided.

@openshift-ci openshift-ci bot requested review from 2uasimojo and mfojtik July 12, 2021 15:31
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2021
Copy link
Member

@2uasimojo 2uasimojo left a comment

Choose a reason for hiding this comment

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

  • Can we add this for imagebuilder as well?
  • This means clean-foo will only clean the configured version of foo. Do we want to address that? Perhaps clean-foo should remove _output/bin/foo*?

@2uasimojo
Copy link
Member

Oh, right, imagebuilder is installed into go, not in the repo :( Never mind.

@2uasimojo
Copy link
Member

This means clean-foo will only clean the configured version of foo. Do we want to address that? Perhaps clean-foo should remove _output/bin/foo*?

I'll propose a fup to discuss this.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 12, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, sttts

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 9e7d294 into openshift:master Jul 12, 2021
2uasimojo added a commit to 2uasimojo/build-machinery-go that referenced this pull request Jul 12, 2021
We provide a `clean-$foo` target for helper tools -- `$foo` in:
- `controller-gen`
- `kustomize`
- `yaml-patch`
- `yq`

Since openshift#48 / aa83de7 we're installing those helpers to files with
versioned names -- e.g. `yq-2.4.0` rather than just `yq` -- so `make
clean-yq` would leave behind other versions of `yq`.

With this commit, `make clean-$foo` will remove `${foo}*`, which should
lasso all previous versions of a given helper, including one with the
unversioned name, which better meets the consumer's expectation of what
"clean $foo" should mean.
2uasimojo added a commit to 2uasimojo/build-machinery-go that referenced this pull request Jul 12, 2021
We provide a `clean-$foo` target for helper tools -- `$foo` in:
- `controller-gen`
- `kustomize`
- `yaml-patch`
- `yq`

Since openshift#48 / aa83de7 we're installing those helpers to files with
versioned names -- e.g. `yq-2.4.0` rather than just `yq` -- so `make
clean-yq` would leave behind other versions of `yq`.

With this commit, `make clean-$foo` will remove `${foo}*`, which should
lasso all previous versions of a given helper, including one with the
unversioned name, which better meets the consumer's expectation of what
"clean $foo" should mean.
2uasimojo added a commit to 2uasimojo/build-machinery-go that referenced this pull request Jul 12, 2021
We provide a `clean-$foo` target for helper tools -- `$foo` in:
- `controller-gen`
- `kustomize`
- `yaml-patch`
- `yq`

Since openshift#48 / aa83de7 we're installing those helpers to files with
versioned names -- e.g. `yq-2.4.0` rather than just `yq` -- so `make
clean-yq` would leave behind other versions of `yq`.

With this commit, `make clean-$foo` will remove `${foo}*`, which should
lasso all previous versions of a given helper, including one with the
unversioned name, which better meets the consumer's expectation of what
"clean $foo" should mean.
2uasimojo added a commit to 2uasimojo/build-machinery-go that referenced this pull request Jul 12, 2021
We provide a `clean-$foo` target for helper tools -- `$foo` in:
- `controller-gen`
- `kustomize`
- `yaml-patch`
- `yq`

Since openshift#48 / aa83de7 we're installing those helpers to files with
versioned names -- e.g. `yq-2.4.0` rather than just `yq` -- so `make
clean-yq` would leave behind other versions of `yq`.

With this commit, `make clean-$foo` will remove `${foo}*`, which should
lasso all previous versions of a given helper, including one with the
unversioned name, which better meets the consumer's expectation of what
"clean $foo" should mean.
2uasimojo added a commit to 2uasimojo/build-machinery-go that referenced this pull request Aug 6, 2021
aa83de7 / openshift#48 added versioning for helper binaries. However, for
kustomize, it assumed the install script would create the binary with
the versioned name, which it doesn't. So consumers trying to use
`$(KUSTOMIZE)`, which will be `_output/tools/bin/kustomize-v4.1.3`, will
fail because it's actually at `_output/tools/bin/kustomize`.

Furthermore, running `make ensure-kustomize` twice will cause the
install script to fail because its target file already exists.

Fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants