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

*: remove GOPATH requirement from the SDK and operator projects #1475

Merged
merged 51 commits into from
Jun 19, 2019
Merged

*: remove GOPATH requirement from the SDK and operator projects #1475

merged 51 commits into from
Jun 19, 2019

Conversation

estroz
Copy link
Member

@estroz estroz commented May 23, 2019 •
edited

Description of the change: uses projutil.GetGoPkg() instead of projutil.CheckAndGetProjectGoPkg(), which is removed. Tests are run outside of $GOPATH/src.

Motivation for the change: see #1457

Closes #1457

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2019
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 23, 2019
@estroz estroz mentioned this pull request May 23, 2019
Closed
@estroz
test/e2e/main_test.go: use testArgs struct for test args; add
bbc2c69 
local-repo flag to specify the local repo path if the SDK is not
in $GOPATH/src/github.com/operator-framework/operator-sdk

test/e2e/memcached_test.go: remove any 'replace' directives for
the SDK repo in memcached-operator's go.mod file before adding one
for the e2e test
@estroz
hack/tests/e2e-{ansible,helm}.sh: remove SDK replace lines in go.mod …
d5be9a3 
…before adding one
@nicolaferraro nicolaferraro mentioned this pull request May 29, 2019
Closed
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2019
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 17, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019
@estroz estroz requested a review from hasbro17 June 17, 2019 17:41
joelanford
joelanford approved these changes Jun 18, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM after addressing comments.

cmd/operator-sdk/migrate/cmd.go Outdated Show resolved Hide resolved
cmd/operator-sdk/new/cmd.go Outdated Show resolved Hide resolved
@hasbro17
Apply suggestions from code review
d540810 
Co-Authored-By: Haseeb Tariq <hasbro17@gmail.com>
@estroz estroz requested a review from hasbro17 June 18, 2019 21:51
lilic
lilic reviewed Jun 19, 2019
View reviewed changes
Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

A couple of suggestions and questions, but overall lgtm. Very much requested by users so thanks for this! 👍

cmd/operator-sdk/main.go Outdated Show resolved Hide resolved
internal/util/projutil/project_util.go Show resolved Hide resolved
internal/util/projutil/project_util.go Outdated Show resolved Hide resolved
internal/util/projutil/project_util.go Outdated Show resolved Hide resolved
Eric Stroczynski and others added 2 commits June 19, 2019 09:03
@lilic
Apply suggestions from code review
a4d1c96 
Co-Authored-By: Lili Cosic <cosiclili@gmail.com>
@estroz
more informative log message
d62b15c
@estroz estroz requested a review from lilic June 19, 2019 16:07
hasbro17
hasbro17 approved these changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making all the changes.

A few last doc changes though:

  • Mention this behavior, i.e ability to work outside the GOPATH, in the CHANGELOG
  • Additionally mention the new flag --repo in the CHANGELOG
  • Update the cli-reference guide for the new flag --repo

@lilic
Copy link
Member

lilic commented Jun 19, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019
@estroz estroz merged commit 757f7a0 into operator-framework:master Jun 19, 2019
@estroz estroz deleted the remove-gopath branch June 19, 2019 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford approved these changes

@hasbro17 hasbro17 hasbro17 approved these changes

@AlexNPavel AlexNPavel Awaiting requested review from AlexNPavel

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

@theishshah theishshah Awaiting requested review from theishshah

@lilic lilic Awaiting requested review from lilic

Assignees

@lilic lilic

Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SDK should work outside of $GOPATH/src
6 participants
@estroz @lilic @openshift-ci-robot @joelanford @painhardcore @hasbro17