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

GCloud state backend: coding style fixes #1

Merged
merged 9 commits into from
Sep 7, 2017

Conversation

octo
Copy link
Collaborator

@octo octo commented Sep 7, 2017

Here are my changes to the GCloud implementation, as mentioned in hashicorp#15592.

Noteworthy changes are:

  1. Return the GCS object's generation as the lock ID. This allows Unlock() to call "delete if generation matches" immediately, removing one round-trip.
  2. Hold on to the context passed to the configure() callback to avoid calling context.Background().

Any feedback is welcome :)

Best regards,
—octo

Copy link
Owner

@pbzdyl pbzdyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like all the changes and I could learn a lot from them (I am new to Go lang). I have only one (cosmetic) question about a code comment.

@@ -58,11 +58,14 @@ func (b *Backend) configure(ctx context.Context) error {
return nil
}

data := schema.FromContextBackendConfig(ctx)
// ctx is a background context with the backend config added.
// Since no context is passed to RemoteClient.Get(), .Lock(), etc. we
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems to be truncated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh, thanks for noticing. I'll update my branch in a second :)

octo added 2 commits September 7, 2017 17:10
Calling context.Background() from outside the main() function is
discouraged. The configure functions are only called from
"…/helper/schema".Backend.Configure which provides the Background context,
i.e. a long-living context we can use for backend communication.
This allows Unlock() to call Delete() without reading the lock file's
content first.
@octo
Copy link
Collaborator Author

octo commented Sep 7, 2017

There, comment is fixed now :)

You mentioned that you're new to Go. Would you prefer if I left review comments to get the code into a more idiomatic Go form as a sort of learning experience or should I continue to just fix it and send PRs?

Best regards,
—octo

@pbzdyl
Copy link
Owner

pbzdyl commented Sep 7, 2017

I would like to slow you down - I can learn from the diffs too :)

Is the PR ready to be merged or are you going to push more fixes to make the code look more idiomatic?

@octo
Copy link
Collaborator Author

octo commented Sep 7, 2017

I'm sure there will be more patches in the future, but that'll be it for today. I'll likely be able to spend some more time on this tomorrow :) Feel free to merge at your convenience, I can always create another PR ;)

@pbzdyl pbzdyl merged commit 0e57c46 into pbzdyl:gcloud-backend Sep 7, 2017
@pbzdyl
Copy link
Owner

pbzdyl commented Sep 7, 2017

Thank you for all the improvements @octo!

By the way, would you like to take a look at changes required to make the new backend compatible with the existing gcs backend?

I will definitely spend some time on it but not earlier than next week.

@octo
Copy link
Collaborator Author

octo commented Sep 7, 2017

Yes, I'll take a look at that tomorrow. Thank you for your PR, it's looking great so far :)

@singhm1994
Copy link

I am facing an issue in which when I try to initialize Terragrunt I am receiving this error Error: Error locking state: Error acquiring the state lock: writing "gs:///default.tflock" failed: googleapi: Error 412: Precondition Failed, conditionNotMet

I am using terraform v0.11.15-oci and Terrafrunt 0.18.7

Request you to assist on the same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants