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

MYST-178. Fixed import path in packages #321

Merged
merged 5 commits into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@soffokl
Copy link
Member

commented Aug 27, 2018

No description provided.

@soffokl soffokl added the enhancement label Aug 27, 2018

@soffokl soffokl self-assigned this Aug 27, 2018

@soffokl soffokl requested review from tadovas and Waldz as code owners Aug 27, 2018

@tadovas

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

This is huge :|

@tadovas

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@Waldz we discussed about this long time ago. I cannot remember what approach or conclusion we made.

.travis.yml Outdated
@@ -2,7 +2,7 @@ sudo: true
os: linux
language: go
go: [1.9.2]
go_import_path: github.com/mysterium/node
go_import_path: github.com/MysteriumNetwork/node

This comment has been minimized.

Copy link
@zolia

zolia Aug 28, 2018

Member

I am not sure about upper cased MysteriumNetwork, apart that it looks bad its again common practice and "not all file systems are case sensitive." (source: https://talks.golang.org/2014/names.slide#16)

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 28, 2018

Author Member

If the file system is case insensitive it will support both paths for import, the upper case will break nothing here.
And here is a comment, that describes that path of the URL is case sensitive since go get use it, it should support it.

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 28, 2018

Member

A. I would go for github.com/mysterium-network/node
or
B. A. Rename our organization to github.com/mysterium-network/node
or
C. Even rename our repo to github.com/mysterium-network/go-node

This comment has been minimized.

Copy link
@zolia

zolia Aug 28, 2018

Member

Working with such repo in case-insensitive env might produce code that will be interpreted in other OS'es as lower case thus braking work flow per-se.

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 28, 2018

Author Member

For me any of A, B or C is good.
I just don't like the way when we need to rename cloned repo to start a build.

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 28, 2018

Author Member

My vote is for github.com/mysterium-network/node.

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 28, 2018

Member

I like github.com/mysteriumnetwork/go-node

This comment has been minimized.

Copy link
@tadovas

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 29, 2018

Member

Do we really want to make organization renaming? That's a bit complicated. Maybe sticking to MysteriumNetwork (case sensitive) would be fine enough

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 5, 2018

Member

I renamed organization. Go ahead with name github.com/mysteriumnetwork/node

@Waldz Waldz changed the title [WIP] MYST-178. Fixed import path in packages WIP MYST-178. Fixed import path in packages Aug 31, 2018

@soffokl soffokl force-pushed the MYST-178/import-path branch from 56cbaaa to 8cb1f68 Sep 5, 2018

soffokl added some commits Sep 5, 2018

@soffokl soffokl force-pushed the MYST-178/import-path branch from 8cb1f68 to 67b189c Sep 5, 2018

soffokl added some commits Sep 5, 2018

@soffokl soffokl changed the title WIP MYST-178. Fixed import path in packages MYST-178. Fixed import path in packages Sep 5, 2018

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

@Waldz @tadovas @zolia this PR is ready for review.

.travis.yml Outdated
@@ -2,7 +2,7 @@ sudo: true
os: linux
language: go
go: [1.9.2]
go_import_path: github.com/mysterium/node
go_import_path: github.com/mysteriumnetwork/node

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

I guess this is not needed on travis anymore

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 5, 2018

Author Member

Looks like this, removed it.

@@ -10,8 +10,8 @@ brew install dep
brew install openvpn
export GOPATH=~/workspace/go
git clone git@github.com:MysteriumNetwork/node.git $GOPATH/src/github.com/mysterium/node
cd $GOPATH/src/github.com/mysterium/node
git clone git@github.com:MysteriumNetwork/node.git $GOPATH/src/github.com/mysteriumnetwork/node

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

What about 'git clone ..MysteriumNetwork..' ? seems like leftover?

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 5, 2018

Author Member

👍 right, fixed too.

@tadovas

tadovas approved these changes Sep 5, 2018

Copy link
Member

left a comment

LGTM. It's just 156 edited files. What could possibly go wrong? :)

@Waldz

Waldz approved these changes Sep 5, 2018

@tadovas tadovas merged commit 69fda2d into master Sep 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tadovas tadovas deleted the MYST-178/import-path branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.