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

provider/openstack Add Distributed router support #4878

Merged
merged 1 commit into from
Feb 17, 2016

Conversation

Fodoj
Copy link

@Fodoj Fodoj commented Jan 28, 2016

Please note that this PR depends on this rackspace/gophercloud#525 to be merged.

@stack72
Copy link
Contributor

stack72 commented Jan 28, 2016

Hi @Fodoj, please can you squash your commits on this one - this will help keep the git commit log clean

thanks

Paul

@jtopjian
Copy link
Contributor

@Fodoj Thanks for this! Just a heads up that I'm currently traveling for work, so I might not get a chance to review this for a few days.

@Fodoj Fodoj force-pushed the support-dvr branch 2 times, most recently from a4457c4 to df4b908 Compare January 29, 2016 08:47
@Fodoj
Copy link
Author

Fodoj commented Jan 29, 2016

@stack72 done, thanks

@jtopjian jtopjian added the wip label Jan 31, 2016
@jtopjian jtopjian changed the title provider/openstack Add Distributed router support [wip] provider/openstack Add Distributed router support Jan 31, 2016
@phinze phinze removed the wip label Feb 9, 2016
@jtopjian
Copy link
Contributor

Looks like rackspace/gophercloud#525 was merged today! Can you trigger the Travis tests to run?

Looks good to merge after that. 😄

Edit: err... looks like something else might need to be done in order to get the new Gophercloud merges in, per #4909

@Fodoj
Copy link
Author

Fodoj commented Feb 10, 2016

@jtopjian it's about pinning gophercloud version, right?

@jtopjian
Copy link
Contributor

Right. You'll have to use godep to manage the versions, but I haven't been able to figure out how to do it cleanly. It looks like GO15VENDOREXPERIMENT=1 (which is explained in the README) is used, but doing:

$ godep update github.com/rackspace/gophercloud

Just clobbers vendor/github/rackspace/gophercloud rather than update it.

@phinze @radeksimko I know you guys said you'll be putting docs together shortly. Any quick pointers on the above in the meantime?

@phinze
Copy link
Contributor

phinze commented Feb 10, 2016

Hi @jtopjian - sorry about the delay on those docs. In the meantime, I believe the target of godep update needs a ... afterwards to capture all the packages in that dir for analysis. Give this a shot:

$ godep update github.com/rackspace/gophercloud/...

@jtopjian
Copy link
Contributor

@phinze No problem at all! I totally understand this is a large change that was just implemented :)

I tried ... and it had the same effect, however, I just realized that Godep is simply stripping out _test.go files. All other updates are correct.

Is this intended behavior? The Godep README states that test files are not tracked, but they currently exist in vendor, so I just want to make sure.

@phinze
Copy link
Contributor

phinze commented Feb 10, 2016

@jtopjian interesting! The original contents of vendor were inadvertently captured with an older version of godep so that's entirely possible. Let me clean that discrepancy up in a separate PR so you don't have to deal with that noise.

@jtopjian
Copy link
Contributor

@phinze sounds like an awesome plan! Thank you!

@Fodoj
Copy link
Author

Fodoj commented Feb 11, 2016

If I run godep update I get "Package (github.com/golang/protobuf/proto) not found

@jtopjian
Copy link
Contributor

@Fodoj Here's what I ended up doing for #5131:

rm -rf ~/go/src/github.com/hashicorp/terraform
go get github.com/hashicorp/terraform
cd ~/go/src/github.com/hashicorp/terraform
git remote add jtopjian ...
git fetch jtopjian
git checkout --track jtopjian/...
git rebase -i master
export GO15VENDOREXPERIMENT=1
godep update github.com/rackspace/gophercloud
git status
git add .
git commit -m "provider/openstack: Updating Gophercloud Dependency"

I have no idea how correct that is... but I think it worked :)

@Fodoj Fodoj changed the title [wip] provider/openstack Add Distributed router support provider/openstack Add Distributed router support Feb 15, 2016
@Fodoj
Copy link
Author

Fodoj commented Feb 15, 2016

@jtopjian ping

@jtopjian
Copy link
Contributor

This looks good to me! Did you end up using the steps I listed?

I'd like @phinze to do a quick review of the Godeps stuff since this will be the first for an OpenStack-related commit.

@Fodoj
Copy link
Author

Fodoj commented Feb 16, 2016

@jtopjian yes, your steps were mostly correct, though I also had to run godep restore -v to pull deps

@phinze
Copy link
Contributor

phinze commented Feb 16, 2016

Godeps diff LGTM!

jtopjian added a commit that referenced this pull request Feb 17, 2016
provider/openstack Add Distributed router support
@jtopjian jtopjian merged commit dc5aa5e into hashicorp:master Feb 17, 2016
@Fodoj Fodoj deleted the support-dvr branch February 17, 2016 15:24
@ghost
Copy link

ghost commented Apr 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants