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

state/remote/swift: Updates #9769

Merged
merged 3 commits into from
Nov 3, 2016

Conversation

fatmcgav
Copy link
Contributor

@fatmcgav fatmcgav commented Nov 1, 2016

Updates to swift remote state support:

  • Use Gophercloud/gophercloud
  • Add support for 'insecure' TLS
  • Add Keystone v3 auth values
  • Documentation updates

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

@jtopjian Not sure if you've done anything with Terraform remote state before, but wouldn't mind a quick review of this one from a Gophercloud POV...

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

Hmm, looks like am missing some vendored packages from gophercloud/gophercloud...

Now lets see if I can work out how to vendor them in :)

@jtopjian
Copy link
Contributor

jtopjian commented Nov 1, 2016

@fatmcgav Nice. I've been meaning to take a look at the Swift remote state support for a while. :)

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

@jtopjian Heh, surprisingly it "Just worked" ;)

Just making some additions now for extra config settings inline with the changes you made in #9725

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

OK, I think I've got all the possible config permutations covered...

@jtopjian Fancy giving it a spin?

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

@stack72 I'm guessing I'm going to need to clean this one up a bit... ;)

Any preference on splitting down, or all in one commit?

@fatmcgav fatmcgav changed the title [WIP] state/remote/swift: Updates state/remote/swift: Updates Nov 1, 2016
@jtopjian
Copy link
Contributor

jtopjian commented Nov 1, 2016

@fatmcgav I'll try this out and report back :)

Re cleanup: This PR might make sense as two or more PRs instead of one. It looks like there are distinct topic differences. For example, adding SSL certs doesn't necessarily mean that moving to the new gophercloud repo has to happen at the same time.

Maybe two PRs? One for the Gophercloud move and one for the SSL updates? I can definitely make sure they are merged shortly within each other.

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

@jtopjian Yeh, can look at splitting it down... As this is also the last bit that's using 'rackspace/gophercloud', so can un-vendor that aswell as part of the Gophercloud move PR...

@jtopjian
Copy link
Contributor

jtopjian commented Nov 1, 2016

@fatmcgav that sounds good to me!

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

@jtopjian FYI, I've split the gophercloud migration into https://github.com/fatmcgav/terraform/tree/state_remote_swift_updates_rework.
I haven't yet updated this branch, as didn't want to break the PR if you're testing... ;)

Can then raise a new PR for the other updates...

@jtopjian
Copy link
Contributor

jtopjian commented Nov 1, 2016

@fatmcgav I won't have time to test until this evening (another 7-ish hours my time) so feel free to break this PR up. You can also have the second PR based on the first and unless any changes are made to the first, the second will merge cleanly. I can then test solely on the second PR. :)

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 1, 2016

@jtopjian Cool, ok... I'll get breaking then :)

@fatmcgav fatmcgav changed the title state/remote/swift: Updates [WIP] state/remote/swift: Updates Nov 1, 2016
@fatmcgav fatmcgav force-pushed the state_remote_swift_updates branch 2 times, most recently from c5578ae to 9cc3149 Compare November 1, 2016 16:48
…e to support insecure TLS, added Keystone v3 auth params

Update swift remote documentation
@fatmcgav fatmcgav force-pushed the state_remote_swift_updates branch from 9cc3149 to 54065ac Compare November 1, 2016 16:55
@fatmcgav fatmcgav changed the title [WIP] state/remote/swift: Updates state/remote/swift: Updates Nov 1, 2016
@jtopjian
Copy link
Contributor

jtopjian commented Nov 2, 2016

@fatmcgav wow - this really needed an update! Keystone v3 wasn't even working. Thanks!

The Swift code looks good to go. A few notes about the vendoring:

  1. Can you update the commit messages to have the format:
vendor: Updating github.com/gophercloud/gophercloud
vendor: Removing github.com/rackspace/gophercloud

Feel free to add a short commit message about each.

  1. To fully add/remove a dependency, do:
$ govendor fetch github.com/gophercloud/gophercloud/...
$ govendor remove github.com/rackspace/gophercloud/...

The trailing ... is literally a trailing three dots.

(Edit: actually updating gophercloud/gophercloud is kind of moot. It'll just make noise in vendor.json)

I suspect you'll end up with a lot more deleted files after that. Looks like you have the honours of fully wiping out the old repo :)

Add github.com/gophercloud/gophercloud/openstack/objectstorage/v1/objects and
github.com/gophercloud/gophercloud/openstack/objectstorage/v1/accounts
@fatmcgav fatmcgav force-pushed the state_remote_swift_updates branch from 54065ac to 937deef Compare November 2, 2016 07:20
All code has now migrated to using github.com/gophercloud/gophercloud!
@fatmcgav fatmcgav force-pushed the state_remote_swift_updates branch from 937deef to ef57001 Compare November 2, 2016 07:21
@fatmcgav
Copy link
Contributor Author

fatmcgav commented Nov 2, 2016

@jtopjian Yeh, was very neglected.

Commit messages updated, and lots more github.com/rackspace/gophercloud file deletions after a govendor remove with trailing ....

@jtopjian
Copy link
Contributor

jtopjian commented Nov 3, 2016

LGTM!

@jtopjian jtopjian merged commit 8665457 into hashicorp:master Nov 3, 2016
@ghost
Copy link

ghost commented Apr 20, 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 20, 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.

3 participants