This repository has been archived by the owner on Aug 15, 2023. It is now read-only.
Add a simple GCS storage impl #3
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
minamijoyo
reviewed
Aug 23, 2022
| gcStorage "cloud.google.com/go/storage" | ||
| ) | ||
|
|
||
| type Client interface { |
There was a problem hiding this comment.
Add GoDoc style comments to all public interfaces, types, and methods.
minamijoyo
reviewed
Aug 23, 2022
minamijoyo
reviewed
Aug 23, 2022
minamijoyo
reviewed
Aug 23, 2022
| if err != nil { | ||
| return fmt.Errorf("failed writing to gcs://%s/%s: %w", a.config.Bucket, a.config.Name, err) | ||
| } | ||
| return w.Close() |
There was a problem hiding this comment.
I think we need to use defer w.Close() to close the Writer on error. Is there any reason to implement it differently than Read?
There was a problem hiding this comment.
It is because:
- We do not care the error returned by
r.Close()becauseio.readAll(r)has already handled its responsibility. In other words, afterio.readAll(r)worked properlly, we do not care thatr.Close()returns error or not. - We do care the error returned by
w.Close(), because it may mean that we failed to write/flush data to storage (GCS in this case). Even whenw.Write(...)completed successfully, it may buffer the written data internally so we cannot say it is successfully saved onto the storage.
For reference, a sample code in the official document handles error in the same manner:
// Write some text to obj. This will either create the object or overwrite whatever is there already.
if _, err := fmt.Fprintf(w, "This object contains text.\n"); err != nil {
// TODO: Handle error.
}
// Close, just like writing a file.
if err := w.Close(); err != nil {
// TODO: Handle error.
}
// Read it back.
r, err := obj.NewReader(ctx)
if err != nil {
// TODO: Handle error.
}
defer r.Close()There was a problem hiding this comment.
Got it.
Though it’s technically possible to return an error from a defer function by using a named return value in general, I don’t have much knowledge about the GCS library implementation. It’s better to follow an official example unless we hit an actual issue.
minamijoyo
reviewed
Aug 23, 2022
|
@KengoTODA Thank you for working on this! I agree with you that it's reasonable to start with the minimum options you need. |
Signed-off-by: Kengo TODA <[email protected]>
Signed-off-by: Kengo TODA <[email protected]>
Signed-off-by: Kengo TODA <[email protected]>
Signed-off-by: Kengo TODA <[email protected]>
|
Thanks for your feedback, @minamijoyo! |
|
@KengoTODA I’ve merged the storage part. Thank you! |
I've created minamijoyo/tfmigrate#103 |
minamijoyo
added a commit
that referenced
this pull request
Aug 15, 2023
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, thanks for sharing so great product with us, and keep it open for contribution! 🙌
Here I want to share my idea to add a simple storage implementation for GCS (Google Cloud Storage).
Unlike minamijoyo/tfmigrate#23, this PR keeps its feature very simple. There are only three configurations, and the credential must be provided via
GOOGLE_APPLICATION_CREDENTIALS. There is no way to bypass the authentication.I confirm that this implementation is working with actual GCS service, you may refer my diff in the
tfmigraterepo to grasp how I edittfmigratecore to integrate.I hope that this PR will be an enough small start for the community, to enjoy daily hacking with GCP.
Thanks for your review! 😄