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

feat: Adding examples for integrationconnectors endpointattachment and managed zone #751

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

balanaguharsha
Copy link
Contributor

@balanaguharsha balanaguharsha commented Oct 21, 2024

Adding examples for integrationconnectors endpointattachment and managed zone

Description

Fixes b/374649808

Note: If you are not associated with Google, open an issue for discussion before submitting a pull request.

Checklist

Readiness

  • Yes, merge this PR after it is approved
  • No, don't merge this PR after it is approved

Style

Testing

Intended location

API enablement

  • If the sample needs an API enabled to pass testing, I have added the service to the Test setup file

Review

  • If this sample adds a new directory, I have added codeowners to the CODEOWNERS file

@balanaguharsha balanaguharsha requested review from a team as code owners October 21, 2024 05:31
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

Copy link

snippet-bot bot commented Oct 21, 2024

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@balanaguharsha balanaguharsha changed the title Adding examples for integrationconnectors endpointattachment and managed zone docs: Adding examples for integrationconnectors endpointattachment and managed zone Oct 21, 2024
@balanaguharsha
Copy link
Contributor Author

Please issue /gcbrun

@balanaguharsha
Copy link
Contributor Author

/gcbrun

1 similar comment
@glasnt
Copy link
Contributor

glasnt commented Oct 22, 2024

/gcbrun

@glasnt
Copy link
Contributor

glasnt commented Oct 22, 2024

The first error these samples encountered (for managed zone):

 Error 400: Service account [email protected] does not exist., badRequest
        	            	
        with google_project_iam_member.default,
        on main.tf line 20, in resource "google_project_iam_member" "default":
        20: resource "google_project_iam_member" "default" {
}

Is it expected that this service account is created when the connectors.googleapis.com API is enabled? Could you instead use a custom service account?

Also checking if these code samples are valid (given you're trying to setup DNS for example.com, I'm not sure if this is something that should be provisionable). If it's not, please provide a test.yaml configured to skip tests.

I'll add comments for code review onto the code itself.

integrationconnectors/endpointattachment/main.tf Outdated Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Outdated Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Outdated Show resolved Hide resolved
integrationconnectors/managedzone/main.tf Outdated Show resolved Hide resolved
@glasnt
Copy link
Contributor

glasnt commented Oct 22, 2024

/gcbrun

* See the License for the specific language governing permissions and
* limitations under the License.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

This sample needs a region tag.

* limitations under the License.
*/

data "google_project" "target_project" {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to have two data.google_project, one needs to have a project_id, otherwise you use the provider's google project (the active project). https://registry.terraform.io/providers/hashicorp/google/latest/docs/data-sources/project

I would suggest adding a comment of the config to help show what you mean (and have a comment if the "test_project" is the "current" project

data "google_project" "test_project" {
}

# [START integrationconnectors_managed_zone_example]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only region tag in this sample; is this intended? Do you want the other resources to be shown in the docs? If so, they also need region tags.

@glasnt glasnt changed the title docs: Adding examples for integrationconnectors endpointattachment and managed zone feat: Adding examples for integrationconnectors endpointattachment and managed zone Oct 23, 2024
@glasnt glasnt added the waiting-response Waiting for issue author to respond. label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-response Waiting for issue author to respond.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants