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.

Sign up for GitHub

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

Jump to bottom

build: use go install instead of depricated go get #187

Merged
merged 4 commits into from
Sep 1, 2022
Merged

build: use go install instead of depricated go get #187

merged 4 commits into from
Sep 1, 2022

Conversation

iamniting
Copy link
Member

@iamniting iamniting mentioned this pull request May 9, 2022
Closed
@iamniting iamniting force-pushed the golang-1.18 branch 3 times, most recently from 73327a4 to 5752dd3 Compare May 9, 2022 14:33
@iamniting iamniting requested review from nbalacha, leelavg and sp98 and removed request for nbalacha and leelavg May 9, 2022 14:34
@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

1 similar comment
@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

leelavg
leelavg reviewed May 10, 2022
View reviewed changes
Makefile
@@ -210,20 +210,11 @@ jsonnet: ## Download jsonnet locally if necessary.
GINKGO = $(shell pwd)/bin/ginkgo
ginkgo: ## Download ginkgo and gomega locally if necessary.
$(call go-get-tool,$(GINKGO),github.com/onsi/ginkgo/v2/ginkgo)
$(call go-get-tool,$(GOMEGA),github.com/onsi/gomega/...)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • is removal of GOMEGA installation intentional?
  • is it due to GINKGO also installing GOMEGA?

Copy link
Member Author

Choose a reason for hiding this comment

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

is removal of GOMEGA installation intentional?
yes it is, because we don't require a binary of GOMEGA and we already have it in our go.mod

nbalacha
nbalacha reviewed May 10, 2022
View reviewed changes
Makefile
@@ -210,20 +210,11 @@ jsonnet: ## Download jsonnet locally if necessary.
GINKGO = $(shell pwd)/bin/ginkgo
ginkgo: ## Download ginkgo and gomega locally if necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ginkgo: ## Download ginkgo and gomega locally if necessary.
ginkgo: ## Download ginkgo locally if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the comment.

@nbalacha
Copy link
Contributor

Can you include this as a part of a PR to use go 1.18?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2022
@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

2 similar comments
@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

@nbalacha
Copy link
Contributor

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 23, 2022
@iamniting
Copy link
Member Author

@nbalacha As discussed over a meet updated the PR, I Will create a new PR for the 1.18 specifically

@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

nbalacha
nbalacha suggested changes May 26, 2022
View reviewed changes
Makefile Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 26, 2022
@riya-singhal31
Copy link
Contributor

/test lvm-operator-bundle-e2e-aws

@riya-singhal31
Copy link
Contributor

Nitin can you please add the missing go.sum entry too in this PR. So that it can pass the e2e.
Ref: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/red-hat-storage_lvm-operator/187/pull-ci-red-hat-storage-lvm-operator-main-lvm-operator-bundle-e2e-aws/1542518880071585792

@iamniting
Copy link
Member Author

/test lvm-operator-bundle-e2e-aws

@iamniting
build: use go install instead of depricated go get
d059996 
ref: https://go.dev/doc/go-get-install-deprecation

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@iamniting
build: update kustomize version
5d0be25 
older kustomize version fails during go install

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@iamniting
build: do not install gomega as a binary
e79ae10 
We are not using gomega as a binary and using it only as a lib which is
already present in go.mod. Also go install fails downloading gomega as
there is no binary.

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2022
@iamniting
fix: add a version for ginkgo
2e140aa 
go install does require a version to be present to download any binary

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>
@iamniting
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022
nbalacha
nbalacha reviewed Sep 1, 2022
View reviewed changes
Makefile
@@ -214,21 +214,12 @@ jsonnet: ## Download jsonnet locally if necessary.

GINKGO = $(shell pwd)/bin/ginkgo
ginkgo: ## Download ginkgo and gomega locally if necessary.
$(call go-get-tool,$(GINKGO),github.com/onsi/ginkgo/v2/ginkgo)
$(call go-get-tool,$(GOMEGA),github.com/onsi/gomega/...)
$(call go-get-tool,$(GINKGO),github.com/onsi/ginkgo/v2/ginkgo@v2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use the latest version available? Currently it is https://github.com/onsi/ginkgo/releases/tag/v2.1.6

Copy link
Member Author

Choose a reason for hiding this comment

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

It is picking up the latest one only as we are mentioning the full version. So whatever is latest in v2 it will pick that up.

$ ./bin/ginkgo version
Ginkgo Version 2.1.6

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, nbalacha, sp98, Yuggupta27

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 00d346b into openshift:main Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leelavg leelavg leelavg left review comments

@sp98 sp98 sp98 approved these changes

@nbalacha nbalacha nbalacha approved these changes

@Yuggupta27 Yuggupta27 Yuggupta27 approved these changes

Assignees

@sp98 sp98

@nbalacha nbalacha

@Yuggupta27 Yuggupta27

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@iamniting @nbalacha @riya-singhal31 @sp98 @Yuggupta27 @leelavg @openshift-merge-robot