-
Notifications
You must be signed in to change notification settings - Fork 57
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
Support for GCS history backend #23
Conversation
4a049e1
to
50bdc27
Compare
@ethernetdan Thank you for working on this!
|
For 1, looking at S3 history storage tests, I wrote only unit tests. It's ok to add integration tests if you want, but I think it's not required to merge this feature. |
@minamijoyo @ethernetdan https://gist.github.com/w1mvy/308b1a9be16b2ec28a7089611c978a01 If there's anything else I can do to help, just let me know. |
@w1mvy thank you for helping debug. I've been just waiting for a response from @ethernetdan. |
@w1mvy Thanks for verifying this! @minamijoyo Sorry for the delay in responding, wanted to make sure the issue wasn't on the tfmigrate side but seems like that's not the case. I'm for merging if you're happy with where it is now. |
@ethernetdan Thank you for your reply. It seems that the Terraform core has a problem with the fake-gcs-server, but as I said before, I think adding acceptance tests for a new storage type is not required to merge. This branch looks stale and conflicts with the latest master branch. Could you rebase it on the master? I'll review it later. |
50bdc27
to
b2618fc
Compare
b2618fc
to
5c32151
Compare
Just rebased, let me know if there's any changes you'd like. |
Thank you both very much. |
Bucket string `hcl:"bucket"` | ||
|
||
// Prefix is the directory where state files will be saved inside the bucket. | ||
Prefix string `hcl:"prefix"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with the real gcs backend and noticed that there is an inconsistent behavior. The prefix
attribute in the tfmigrate gcs storage doesn't actually work as prefix and looks just a name of object. It saves a history file as prefix
, on the other hand the prefix attribute in the terraform gcs backend saves a state file as prefix/default.tfstate
. I guess default
is a name of workspace, right?
Ideally it should behave the same, but referencing the workspace name from the current storage implementation is not so easy. As a compromise, how about just using the name
attribute so that users doesn't expect the same behavior?
|
||
// EncryptionKey is a 32 byte base64 encoded 'customer supplied encryption key' used to encrypt all state. | ||
EncryptionKey string `hcl:"encryption_key,optional"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documents for gcs storage type in README.
https://github.com/minamijoyo/tfmigrate/tree/v0.2.7#storage-block
} | ||
|
||
// adapted from https://github.com/hashicorp/terraform/blob/2635b3b/backend/remote-state/gcs/backend.go#L113-L228 | ||
func (s *GCSStorage) init(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code you copied from the upstream violates the MPL2.0 license. The upstream hashicorp/terraform is distributed under the terms of MPL2.0, but the tfmigrate is currently distributed under MIT.
I understand that tfmigrate users will expect to be authenticated cloud providers with the same options and precedences as terraform backend. This was not a problem in the s3 storage (I think) because the authentication logic for aws is implemented in an external library hashicorp/aws-sdk-go-base.
Can I use your patch as MPL2.0? If you agree it, I'll refactor and split storage related code into a new package to clarify that the code in the new package uses MPL2.0. If you don't agree it, I (or someone else) need to create a new external library with MPL2.0, this will need additional works. I would like to confirm your thoughts before moving this forward.
actions = [ | ||
"mv null_resource.foo2 null_resource.bar2" | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this file? Is it enough mv_bar1.hcl
?
history { | ||
storage "gcs" { | ||
bucket = "tfmigrate-gcs" | ||
prefix = "tfmigrate/history.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align the indent
@ethernetdan Thank you for your work! I reviewed this PR and left some comments. Please check them. |
The gcs history storage has been finally added in v0.3.7 via #103. Thank you all for working on this! |
I originally wrote the local and s3 storage implementation for tfmigrate as the MIT license. Next, someone sent me a patch for gcs storage implementation minamijoyo/tfmigrate#23, copied from the upstream terraform backend implementation. I was concerned it would violate the MPL2 license because the tfmigrate' license is MIT, and terraform's is MPL2. So I split the storage implementations minamijoyo/tfmigrate#80 into a separate https://github.com/minamijoyo/tfmigrate-storage repository and relicensed it as MPL2 so that someone could easily copy snippets from the upstream. However, we could not contact that patch's author and the initial patch was not merged finally. Time passed, KengoTODA sent me another patch #3 for gcs support which contained minimal functionality and was not copied from upstream. This patch has finally been accepted and merged. That is, I've changed the license of the storage implementation with the intention that it will contain a derivative work of terraform that can be easily copied from terraform's backend implementation to extend various storage types, but this has yet to happen. It's clearly far from the terraform's implementation at the time of writing. Recently, HashiCorp announced that it would change the license of its products, including Terraform, from MPL2 to BSL1.1. https://www.hashicorp.com/blog/hashicorp-adopts-business-source-license If, as I originally intended, we allow contributors to copy code from upstream terraform, the tfmigrate-storage repository must also be changed to BSL. This means that the tfmigrate source can continue to be distributed as MIT, but the tfmigrate's binary will be bounded by BSL. Even though HashiCorp's BSL is a reasonably permissive license with no restrictions other than HashiCorp's competitors, it is no longer open-source by OSI definition. I want to continue to keep tfmigrate as an open-source project. Technically, it is possible to keep the tfmigrate-storage repository as MPL2. Still, I no longer have a solid reason to do so; it is just inconvenient to have a separate repository. Fortunately, KengoTODA and I are still the only contributors to the tfmigrate-storage repository, and I got permission from KengoTODA to re-license his patch as MIT in minamijoyo/tfmigrate#146. So, I'm going to change the license of the tfmigrate-storage repository to MIT and will merge it into the tfmigrate repository.
This PR attempts to add support for Google Cloud Storage (GCS) as a backend for tfmigrate history. Process went pretty well but had some questions about testing and hit an issue that I could use some help on.
Planning to add integration tests but wanted to get your thoughts before proceeding. It doesn't appear that there's a localstack equivalent for Google Cloud that mocks the entire platform. My initial thought is use fake-gcs-server to mock just storage. What do you think?
Currently debugging an issue setting up
test-fixtures/storage_gcs
. It appears to setup the backend correctly but seems to expect that the migration already has been applied:Any thoughts on where I might look to troubleshoot?