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

add description field to service_account for Terraform #2435

Merged

Conversation

tmshn
Copy link
Contributor

@tmshn tmshn commented Oct 6, 2019

I've added the description field to resource google_service_account

ref:

Release Note Template for Downstream PRs (will be copied)

`The length of the `description` field of `google_service_account` is now limited to 256 characters.

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@tysen, please review this PR or find an appropriate assignee.

@tmshn
Copy link
Contributor Author

tmshn commented Oct 29, 2019

@ndmckinley Should I close this PR due to #2513 ?

@nat-henderson
Copy link
Contributor

Yours is definitely the better version of this PR. I'll let @tysen decide what to do - maybe we should undo that PR and merge this one instead?

@tmshn
Copy link
Contributor Author

tmshn commented Oct 30, 2019

@ndmckinley As another choice; I'm happy to merge master and resolve conflicts between our codes ( time.Sleep(time.Second) is something missing in my code).

@tysen
Copy link

tysen commented Oct 30, 2019

As another choice; I'm happy to merge master and resolve conflicts between our codes ( time.Sleep(time.Second) is something missing in my code).

@tmshn - That sounds like a good plan to me.

@tmshn
Copy link
Contributor Author

tmshn commented Oct 31, 2019

@tysen resolved 3b1671c

@tysen
Copy link

tysen commented Nov 5, 2019

Looks like there's some errors in the build.

@tmshn
Copy link
Contributor Author

tmshn commented Dec 7, 2019

Looks like there's some errors in the build.

@tysen Sorry I didn't notice that! That's just a typo, now fixed: 1798ab1

@tmshn
Copy link
Contributor Author

tmshn commented Dec 7, 2019

And I solved the conflict with #2650 by @ndmckinley (actually his new logic is almost equivalent to mine, so the diff against master is getting smaller).

}).Do()
if err != nil {
return err
if len(updateMask) > 0 {
Copy link

Choose a reason for hiding this comment

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

What's the motivation behind these changes to ..Update? I'm not sure the Terraform SDK would call into ..Update without a diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... this happened during merging #2650 by @ndmckinley into my original code 81c263d, but well you are right, we don't need this.

Copy link
Contributor Author

@tmshn tmshn Dec 10, 2019

Choose a reason for hiding this comment

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

fixed c979aa1

@tmshn tmshn force-pushed the service-account-description branch 2 times, most recently from 6c94752 to c979aa1 Compare December 10, 2019 03:41
@modular-magician
Copy link
Collaborator

modular-magician commented Dec 10, 2019

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, c979aa1.

Pull request statuses

No diff detected in terraform-google-conversion.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.

Copy link

@tysen tysen left a comment

Choose a reason for hiding this comment

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

This validation isn't strictly correct - the docs say "Must be less than or equal to 256 UTF-8 bytes" (bytes, not characters) but I think it's still useful given that 256 single byte characters is the upper bound for what the API would accept.

I'll merge this shortly.

@tysen
Copy link

tysen commented Dec 10, 2019

Merging failed - @tmshn do you mind rebasing?

@tmshn tmshn force-pushed the service-account-description branch from c979aa1 to 5b31ba9 Compare December 30, 2019 06:10
@tmshn
Copy link
Contributor Author

tmshn commented Dec 30, 2019

@tysen

This validation isn't strictly correct - the docs say "Must be less than or equal to 256 UTF-8 bytes" (bytes, not characters)

len against string in Golang returns bytes count, which means valiation.StringLenBetween validates based on bytes count rather than characters count. So I think the validation is correct, isn't it? (maybe I should change the text for release note?)

Merging failed - @tmshn do you mind rebasing?

Rebased (commit history is now cleaned).
(I should have abandoned this PR earlier and open new one only to add validation...sorry to make the situation complicated.)

@tmshn tmshn force-pushed the service-account-description branch from 5b31ba9 to 6ce8bbb Compare December 30, 2019 06:22
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 6ce8bbb.

Pull request statuses

No diff detected in terraform-google-conversion.
No diff detected in Ansible.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1646
depends: hashicorp/terraform-provider-google#5409
depends: modular-magician/inspec-gcp#319

tmshn and others added 3 commits January 15, 2020 00:07
(setting empty value tend to be more buggy than setting other values)
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
@modular-magician modular-magician force-pushed the service-account-description branch from 6ce8bbb to 9070e95 Compare January 15, 2020 00:07
@modular-magician modular-magician merged commit 2b3126e into GoogleCloudPlatform:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants