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

IAP brand & client #3135

Merged
merged 4 commits into from
Mar 11, 2020
Merged

IAP brand & client #3135

merged 4 commits into from
Mar 11, 2020

Conversation

drebes
Copy link
Member

@drebes drebes commented Feb 17, 2020

Fixes hashicorp/terraform-provider-google#1287

Release Note Template for Downstream PRs (will be copied)

`google_iap_brand`
`google_iap_client`

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 923 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 9 files changed, 923 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 118 insertions(+))

}

resource "google_iap_brand" "project_brand" {
support_email = "[email protected]"
Copy link
Member Author

Choose a reason for hiding this comment

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

This acceptance test won't succeed unless the specified group is owned by the service account running the test.

@drebes
Copy link
Member Author

drebes commented Feb 17, 2020

@danawillow
Copy link
Contributor

drive-by: the "fixes" statement won't automatically close the issue if it's in a comment- it needs to be in the PR description or a commit (I usually do the PR description like in #3063)

products/iap/api.yaml Outdated Show resolved Hide resolved
products/iap/api.yaml Outdated Show resolved Hide resolved
third_party/terraform/tests/resource_iap_brand_test.go Outdated Show resolved Hide resolved
products/iap/api.yaml Show resolved Hide resolved
products/iap/api.yaml Outdated Show resolved Hide resolved
products/iap/api.yaml Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 938 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 9 files changed, 938 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 118 insertions(+))

@pvillard31
Copy link

Sorry for the comment/question, feel free to redirect me to the right place if I should ask this somewhere else.

If I correctly understand this PR, its purpose is to provide a way to create the "APIs & Services / OAuth consent screen". However, I do not see how to specify information such as authorised domains, homepage link, etc. Am I missing something?

Also I see that client ID and client secret are made available but how to specify "Authorised JavaScript origins" and "Authorised redirect URIs". Is it out of scope of this PR?

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 939 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 9 files changed, 939 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 118 insertions(+))

@slevenick
Copy link
Contributor

#3162 Should fix the issues I ran into around the beta provider and base path names.

I'm happy with the current implementation, I would just like to get integration tests working for this so that we can run them via CI.

I think all we need is to add a new environment variable to provider_test.go that will represent the organization domain. Then we can depend on support@{org-domain} existing and the service account running the tests to be an admin for that group email (I'll go set this up manually)

@slevenick
Copy link
Contributor

Hey @drebes do you think you will have a chance to add support for those tests via the environment variable sometime soon?

I can also pick this up and add the changes to your branch, just let me know if you want me to do that

@drebes
Copy link
Member Author

drebes commented Mar 9, 2020

Sure I can add them (sorry, I didn't understand "all we need is to add a new environment variable" to mean me, but I can look into it.)

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 10 files changed, 950 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 10 files changed, 950 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 118 insertions(+))

@drebes
Copy link
Member Author

drebes commented Mar 9, 2020

Added the configurable domain for the acceptance tests.

You need to create a group "support@<acceptance_test_domain>" and make the acceptance test service account an owner of such group, and then set GOOGLE_ORG_DOMAIN as <acceptance_test_domain>.

@slevenick
Copy link
Contributor

Awesome, looks good! I got it to pass locally, so I'll modify the CI test org to add this. Can you rebase this from master, looks like a couple things are out of sync

@drebes
Copy link
Member Author

drebes commented Mar 10, 2020

There you go. Rebased with the collapsed sidebar changes that were causing the conflict. Which also made me realized that I hadn't updated the sidebar for iap_client since I had updated it to the short form. Should be good now.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 10 files changed, 950 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 10 files changed, 950 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 118 insertions(+))

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Thanks for the additions!

@slevenick slevenick merged commit e9be1bf into GoogleCloudPlatform:master Mar 11, 2020
@drebes drebes deleted the brands branch March 13, 2020 20:52
nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020
* IAP brand & client

* First pass at review

* get rid of extra_schema entry

* Make GOOGLE_ORG_DOMAIN a test variable
@roaks3 roaks3 mentioned this pull request May 2, 2023
5 tasks
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.

Resource to create OAuth 2.0 credentials
6 participants