-
Notifications
You must be signed in to change notification settings - Fork 151
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
deps: uplift crd-ref-docs to 0.1.0 #1206
deps: uplift crd-ref-docs to 0.1.0 #1206
Conversation
- their release changed and support different os/arch binary Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Makefile
Outdated
ARCH=x86_64 | ||
else | ||
ARCH=$(strip $(shell go env GOARCH)) | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their OS and ARCH values are uname -s
and uname -m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i rewrite this whole shabnag
Makefile
Outdated
test -s $(CRD_REF_DOCS) || curl -sSLo $(CRD_REF_DOCS) $(CRD_REF_DOCS_DL_URL) && \ | ||
chmod +x $(CRD_REF_DOCS) ;\ | ||
test -s $(CRD_REF_DOCS) || ( \ | ||
OS=$(shell go env GOOS) && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not make variable?
Makefile
Outdated
OS=$(shell go env GOOS) && \ | ||
wget -q -O $(LOCALBIN)/$(CRD_REF_DOCS_TARBALL) https://github.com/elastic/crd-ref-docs/releases/download/v$(CRD_REF_DOCS_VERSION)/crd-ref-docs_$(CRD_REF_DOCS_VERSION)_$${OS}_${ARCH}.tar.gz && \ | ||
tar -xzf $(LOCALBIN)/$(CRD_REF_DOCS_TARBALL) -C $(LOCALBIN) && \ | ||
rm -rf $(LOCALBIN)/$(CRD_REF_DOCS_TARBALL) $(LOCALBIN)/LICENSE $(LOCALBIN)/README.md && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use something like
curl -sSL <tarball url> | tar -zxf - -C $(LOCALBIN) crd-ref-docs
Makefile
Outdated
chmod +x $(CRD_REF_DOCS) ;\ | ||
test -s $(CRD_REF_DOCS) || ( \ | ||
OS=$(shell go env GOOS) && \ | ||
wget -q -O $(LOCALBIN)/$(CRD_REF_DOCS_TARBALL) https://github.com/elastic/crd-ref-docs/releases/download/v$(CRD_REF_DOCS_VERSION)/crd-ref-docs_$(CRD_REF_DOCS_VERSION)_$${OS}_${ARCH}.tar.gz && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$()
for make variables please
amd64 is a legacy name from the times when Intel did not support it and promoted their VLIW IA64. Debian keeps it the same for the same reasons. But nowadays name is x86_64. |
Makefile
Outdated
chmod +x $(CRD_REF_DOCS) ;\ | ||
test -s $(CRD_REF_DOCS) || ( \ | ||
curl -sSL https://github.com/elastic/crd-ref-docs/releases/download/v$(CRD_REF_DOCS_VERSION)/crd-ref-docs_$(CRD_REF_DOCS_VERSION)_$(shell uname -s)_$(shell uname -m).tar.gz | tar -xzf - -C $(LOCALBIN) && \ | ||
rm -rf $(LOCALBIN)/LICENSE $(LOCALBIN)/README.md && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put filename to tar to extract (as in my example), then you do not need to rm anything.
Makefile
Outdated
test -s $(CRD_REF_DOCS) || curl -sSLo $(CRD_REF_DOCS) $(CRD_REF_DOCS_DL_URL) && \ | ||
chmod +x $(CRD_REF_DOCS) ;\ | ||
test -s $(CRD_REF_DOCS) || ( \ | ||
curl -sSL https://github.com/elastic/crd-ref-docs/releases/download/v$(CRD_REF_DOCS_VERSION)/crd-ref-docs_$(CRD_REF_DOCS_VERSION)_$(shell uname -s)_$(shell uname -m).tar.gz | tar -xzf - -C $(LOCALBIN) && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have strong opinion, usually I would put that in variables, then the names are self descriptive. But this will work as well.
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ykaliuta 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 |
3ce4d58
into
opendatahub-io:incubation
* deps: uplift crd-ref-docs to 0.1.0 - their release changed and support different os/arch binary Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rewrite after review Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 3ce4d58)
* deps: uplift crd-ref-docs to 0.1.0 - their release changed and support different os/arch binary Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rewrite after review Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 3ce4d58)
* deps: uplift crd-ref-docs to 0.1.0 - their release changed and support different os/arch binary Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rewrite after review Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 3ce4d58)
* deps: uplift crd-ref-docs to 0.1.0 - their release changed and support different os/arch binary Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rewrite after review Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 3ce4d58)
* deps: uplift crd-ref-docs to 0.1.0 - their release changed and support different os/arch binary Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: rewrite after review Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 3ce4d58)
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria