-
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
adding in IAP Tunnel API to the beta provider #1719
adding in IAP Tunnel API to the beta provider #1719
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. |
@rileykarson any updates/can I get a reviewer? |
Ah yup, I was just about to start taking a look. |
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.
Generally looking good! Left some comments inline- mainly, I'm curious why we're using the project # and I think a lot of scaffolding in the tests can be eliminated.
third_party/terraform/tests/resource_iap_tunnel_iam_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_iap_tunnel_iam_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_iap_tunnel_iam_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_iap_tunnel_iam_test.go.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_iap_tunnel_iam_test.go.erb
Outdated
Show resolved
Hide resolved
Working around the project number was a pain. This is due to the IAP API doing things inconsistent with the rest of the google cloud APIs. Check out this page, and flip the toggle from console to API: https://cloud.google.com/iap/docs/using-tcp-forwarding#ssh_tunneling Specifically: You'll see the API path is: |
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.
Generally looks good- some spacing got changed, so ideally we'll preserve the old tab indentation & we need to add docs similar to hashicorp/terraform-provider-google#3551
@rileykarson is there a linter or some sort pre flight/commit script that the team uses? |
third_party/terraform/website/docs/r/compute_instance_iam.html.markdown
Outdated
Show resolved
Hide resolved
third_party/terraform/website/docs/r/google_iap_tunnel_instance_iam.markdown
Show resolved
Hide resolved
We lint the provider, but have no linting MM-side. If you generate into a provider repo locally and then run |
Ah- the tests during generation are failing because we need to update the dependencies in the beta repo. Sorry about that! We just had to remove the automatic vendoring behaviour because it slowed down builds by a lot. What you'll need to do is;
Go Module vendoring behaviour requires us to have code actually using the dependency present to vendor the code necessary, and since we're making changes in two parts we need to trick it by generating locally and only staging the vendor changes. |
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. |
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.
Thanks for your contribution @reechar-goog!
Ah, looks like our merge script hit a speed bump during the merge step; it performs a rebase followed by a squash, so sometimes it gets unhappy with interim commits and I think the spacing changes are conflicting with #1740. Do you mind squashing the changes into a single commit, and rebasing off (If you're uncomfortable with rebasing, just let me know and I can perform it instead.) |
068b088
to
6c4f479
Compare
there seemed to be some funny merging during the rebasing. But I did squash all the commits down, hopefully the force push doesn't make things too angry. |
Rebase + force-pushing (to feature branches exclusively!) is generally our practice on the MM repo, merges are actually generally the ones that make it angry. |
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. |
All of those are spurious downstreams and can be ignored; it looks like we hit 2 different PRs whose patches were applied and then the MM PR merged at once. |
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
This is an implementation for the google-beta terraform provider. This adds in a new beta API (https://godoc.org/google.golang.org/api/iap/v1beta1). It isn't a full implementation of the full API, but it does add support for this feature/workflow: https://cloud.google.com/iap/docs/using-tcp-forwarding.
This addresses this open issue: hashicorp/terraform-provider-google#2916
[all]
[terraform]
[terraform-beta]
[ansible]
[inspec]