Dataplex lake iam #692
Dataplex lake iam #692
Conversation
Signed-off-by: Chloé CARAYON <[email protected]>
Signed-off-by: Chloe CARAYON <[email protected]>
Chloe-Carayon
requested review from
ulucinar,
sergenyalcin and
turkenf
as code owners
There was a problem hiding this comment.
Hey @Chloe-Carayon,
Congratulations on your first contribution! Your PR description is very clear, and you’ve shared the test results in great detail. Thank you for this thoughtful contribution!
Could you please review the discussion in this thread and update your PR by removing the *_iam_binding, *_iam_member resources?
Signed-off-by: Chloe CARAYON <[email protected]>
Chloe-Carayon
force-pushed
the
dataplex_lake_iam
branch
from
a6cdbf6 to
6393c90
Compare
Signed-off-by: Chloe CARAYON <[email protected]>
There was a problem hiding this comment.
Hi @Chloe-Carayon,
I left a few review comments for you to consider. It would be great if you could test the latest status after making the changes and update the PR description. Thank you!
config/externalname.go
Outdated
| @@ -426,6 +426,8 @@ var terraformPluginSDKExternalNameConfigs = map[string]config.ExternalName{ | |||
| // | |||
| // Imported by using the following format: projects/{{project}}/locations/{{location}}/lakes/{{name}} | |||
| "google_dataplex_lake": config.TemplatedStringAsIdentifier("name", "projects/{{ .setup.configuration.project }}/locations/{{ .parameters.location }}/lakes/{{ .external_name }}"), | |||
| // Imported by using the following format: projects/{{project}}/locations/{{location}}/lakes/{{lake}} | |||
| "google_dataplex_lake_iam_policy": config.TemplatedStringAsIdentifier("", "projects/{{ .setup.configuration.project }}/locations/{{ .parameters.location }}/lakes/{{ .external_name }}"), | |||
There was a problem hiding this comment.
When importing this source we use the format .......... /lakes/{{lake}} and we have the lake parameter, so it would be safer to use the following configuration.
| "google_dataplex_lake_iam_policy": config.TemplatedStringAsIdentifier("", "projects/{{ .setup.configuration.project }}/locations/{{ .parameters.location }}/lakes/{{ .external_name }}"), | |
| "google_dataplex_lake_iam_policy": config.TemplatedStringAsIdentifier("", "projects/{{ .setup.configuration.project }}/locations/{{ .parameters.location }}/lakes/{{ .parameters.lake }}"), |
Also here the location parameter is optional but since we use this parameter in external name configuration it would be better to mark it as required, example configuration:
There was a problem hiding this comment.
Thank you, I updated the configuration, regenerate the resources and tested e2e.
| kind: Lake | ||
| metadata: | ||
| annotations: | ||
| meta.upbound.io/example-id: dataplex/v1beta1/lake |
There was a problem hiding this comment.
Could you please add a manual-intervention annotation here as well?
| my-second-lake: exists | ||
| location: us-central1 | ||
| --- | ||
| apiVersion: dataplex.gcp.upbound.io/v1beta1 |
There was a problem hiding this comment.
nit: In our examples' YAML files, we put the main resource at the beginning. Not a blocker, but it would be good.
There was a problem hiding this comment.
I update the example thanks :) !
Signed-off-by: Chloe CARAYON <[email protected]>
Signed-off-by: Chloe CARAYON <[email protected]>
Description of your changes
This PR add new resources for Dataplex Lake IAM: LakeIAMPolicy.
Fixes: #687
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
I run manual tests as we are creating IAM policies for existing users so I commented all
upjet.upbound.io/manual-interventionannotations.LakeIAMPolicy
Results:
