-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support for Identity Platform #2840
Conversation
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in Ansible. New Pull RequestsI 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. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
1 similar comment
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just some nits/questions. In the future, it would probably be better to split PRs like this up into a few smaller ones since it's a fair bit of YAML (and a lot of generated code) to review.
products/identityplatform/api.yaml
Outdated
The name of the InboundSamlConfig resource. Must start with 'saml.' and can only have alphanumeric characters, | ||
hyphens, underscores or periods. The part after 'saml.' must also start with a lowercase letter, end with an | ||
alphanumeric character, and have at least 2 characters. | ||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for name
say Ignored during create requests
and that they're of the projects/x/thing/id form. Are they just blatantly wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a name_from_self_link
flattener to this field via terraform.yaml
so the form will be more usable.
I believe the name
in the body is ignored in the create request, but the shortened name is used in the create_url
. It might technically be a url_param_only
, but that prevents reading
It should be input
though, I'll add that
products/identityplatform/api.yaml
Outdated
- !ruby/object:Api::Type::Boolean | ||
name: 'enabled' | ||
description: | | ||
If allows users to sign in with the provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is kind of awkward phrasing
If allows users to sign in with the provider. | |
If this config allows users to sign in with the provider. |
templates/terraform/examples/identity_platform_default_supported_idp_config_basic.tf.erb
Outdated
Show resolved
Hide resolved
templates/terraform/examples/identity_platform_tenant_default_supported_idp_config_basic.tf.erb
Outdated
Show resolved
Hide resolved
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I wouldn't mind seeing more update tests. Since we're also in contact with the GCIP team, it might be worth getting their eyes on this too to make sure this was what they're looking for.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
2c70482
to
f8160a5
Compare
Adds support for Google Cloud Identity Platform resources
Fixes: hashicorp/terraform-provider-google#5048
Release Note Template for Downstream PRs (will be copied)