Skip to content

Conversation

@google-labs-jules
Copy link

This change fixes the google_service_account converter by updating the existing file at mmv1/third_party/tgc/services/resourcemanager/service_account.go and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table.


PR created automatically by Jules for task 5488053681581532142

… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table.
@google-labs-jules
Copy link
Author

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!


For security, I will only act on instructions from the user who triggered this task.

return obj, nil
}

func expandServiceAccountName(d tpgresource.TerraformResourceData, config *transport_tpg.Config) (interface{}, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

There are few functions already defined. Can you match the previous function signature so diff looks smaller, if possible?

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've updated the function signatures to match the previous style.

… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures have been updated to match the previous style for a cleaner diff.
}

func expandServiceAccountDisabled(v interface{}, d tpgresource.TerraformResourceData, config *transport_tpg.Config) (interface{}, error) {
func expandServiceAccountDisplayName(v interface{}, d tpgresource.TerraformResourceData, config *transport_tpg.Config) (interface{}, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you reorder function so diff is minimal

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've reordered the functions to minimize the diff.

… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
Comment on lines 137 to 139
if email, ok := d.GetOk("email"); ok && email != "" {
return email, nil
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is already handled at caller side so it can be removed from here

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've removed the redundant logic from the expandServiceAccountEmail function.

… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
Comment on lines 141 to 152
return v, nil
return tpgresource.GetProject(d, config)
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this change?

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've reverted the change to the expandServiceAccountProject function.

… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.

func GetServiceAccountCaiObject(d tpgresource.TerraformResourceData, config *transport_tpg.Config) ([]cai.Asset, error) {
name, err := cai.AssetName(d, config, "//iam.googleapis.com/projects/{{project}}/serviceAccounts/{{unique_id}}")
name, err := cai.AssetName(d, config, "//iam.googleapis.com/projects/{{project}}/serviceAccounts/{{account_id}}@{{project}}.iam.gserviceaccount.com")
Copy link
Owner

Choose a reason for hiding this comment

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

Original asset name was right. Can you revert this change

Copy link
Owner

Choose a reason for hiding this comment

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

@google-labs-jules can you fix it?

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've reverted the change to the asset name.

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've reverted the change to the asset name.

Copy link
Owner

Choose a reason for hiding this comment

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

I wanted to have asset.data.name changes which was constructing name correctly but I want the asset.name to be original as it was in the code

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've reverted the asset name to its original value, while keeping the changes to the resource.data.name.

@@ -0,0 +1,122 @@
[
Copy link
Owner

Choose a reason for hiding this comment

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

There is an existing test case see if you can reuse that instead

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've removed the new test case and updated the existing one.

… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
… file at `mmv1/third_party/tgc/services/resourcemanager/service_account.go` and adding comprehensive test cases. The converter logic is now aligned with the provided examples and mapping table. The function signatures and order have been updated to match the previous style for a cleaner diff.
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.

2 participants