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

AppEngine Standard Application Version resource. #1884

Merged
merged 3 commits into from
Aug 26, 2019

Conversation

khanali21
Copy link
Contributor

@khanali21 khanali21 commented Jun 5, 2019

This commit adds the AppEngine standard application version resource.
This is work in progress, however I would like to seek initial review from the contributors.

Release Note for Downstream PRs (will be copied)

New output-only fields have been added to google_cloud_run_service' status.

@modular-magician
Copy link
Collaborator

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.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of fae29170cb50ebdc56dd238d8b34578f7bd1f295.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I 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.
depends: hashicorp/terraform-provider-google-beta#817
depends: GoogleCloudPlatform/terraform-google-conversion#98
depends: hashicorp/terraform-provider-google#3799

@khanali21
Copy link
Contributor Author

Is this conflict build/terraform-beta expected?

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Good job! Looks like you're on the right track. I have a handful of comments, primarily about convention. They should help resolve some issues as you get to integration testing, particularly the import stage during tests which is a bit fiddly about resource id's.

Also, make sure to add descriptions to all the properties (this is just copied from the GCP rest reference most of the time). As you can see in the current downstreams generated by the Magician, those make up the user-facing documentation for the fields.

The build/ conflict is expected, generally we don't commit changes to the build/ dir. Personally, I keep mine disable with git submodule deinit. I think you can use some combination of git reset and git checkout to revert them to their state at master.

products/appengine/api.yaml Outdated Show resolved Hide resolved
products/appengine/api.yaml Show resolved Hide resolved
products/appengine/terraform.yaml Outdated Show resolved Hide resolved
products/appengine/api.yaml Outdated Show resolved Hide resolved
products/appengine/api.yaml Show resolved Hide resolved
products/appengine/api.yaml Show resolved Hide resolved
products/appengine/api.yaml Show resolved Hide resolved
products/appengine/api.yaml Outdated Show resolved Hide resolved
@khanali21
Copy link
Contributor Author

Hi,
Why the concourse-ci/status is not getting reported? Is there anything I need to do? Please advise.
Thanks in advance.

@rileykarson
Copy link
Member

Ah, triggering Concourse builds is a manual process for external contributions, once a member of the team has looked over the code briefly. I didn't notice this getting pushed, it didn't trigger a notification for some reason. If you push and want me to take a look, include a comment as well! Those should always result in a notication.

It looks like there's merge conflicts in the build/ folders. I started a build, but I expect they'll cause it to fail. Can you rebase on top of master and update those folders so there's no conflict?

@rileykarson
Copy link
Member

Looks like the build failed because of Ansible, which has a required field for supported resources. Can you add an exclusion for this resource in appengine/ansible.yaml? Similar to https://github.com/GoogleCloudPlatform/magic-modules/blob/master/products/compute/ansible.yaml#L62-L63

@rileykarson
Copy link
Member

Oops, sorry for the extra round trip- can you do the same for Service as well?

@khanali21
Copy link
Contributor Author

No my fault should have checked the api.yml for any other resource. Many thanks for your support @rileykarson

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 3565fbb.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

This is a massive resource (the generated downstream is >4000 lines), and ensuring that we've codified the fields correctly will be extremely difficult. What do you think of breaking this into a few phases, adding more configurable fields progressively? That will help keep the size of your changeset down, and help me provide a meaningful review of the resource.

We could potentially release a partially-featured version of the resource sooner as well given that approach.

https://cloud.google.com/appengine/docs/standard/python/getting-started/python-standard-env seems like a good feature set for a first phase. After creating the files in that tutorial and running gcloud app deploy --log-http, gcloud sent this request and got this response back: https://gist.github.com/rileykarson/0fd78db9dd7c6811a4f59d12924cf807

I bet we can make a first pass of this resource supporting only:

  • deployment
    • files
  • entrypoint
  • handlers
    • script
      • scriptPath
    • securityLevel
    • urlRegex
  • id
  • libraries
    • name
    • version
  • runtime
  • runtimeApiVersion
  • threadsafe

While there are additional fields in the response, we don't necessarily need them initially, so we can omit them. Once we've merged a PR containing that subset of fields, adding more fields can be done in small sets that are much more manageable and we can iterate on more quickly.


resource "google_app_engine_app_version" "<%= ctx[:primary_resource_id] %>" {
version_id = "v31"
project = "${google_app_engine_application.app.project}/services/default"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
project = "${google_app_engine_application.app.project}/services/default"
project = "${google_app_engine_application.app.project}"
service = "default"

I think this is what you want? https://github.com/terraform-providers/terraform-provider-google-beta/pull/817/files#diff-be7a1b17548460da485ebadb23552368R846 will form the request URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, in fact I had started only with files initially and then added Zip and other fields. I would push the changes as suggested..

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, c8f2877.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Generated downstreams and attempted to run the generated test, I noticed a syntax error in it.

products/appengine/api.yaml Outdated Show resolved Hide resolved
products/appengine/api.yaml Outdated Show resolved Hide resolved
products/appengine/api.yaml Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, a003183.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@khanali21
Copy link
Contributor Author

@rileykarson Hi Riley, Is there anything I need to do? Please let me know.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot removed the cla: no label Jul 23, 2019
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 20ca8b5.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

LGTM!

There's a bug in our generation process that assigns commits in downstream PRs to the user's primary email address and not their original commit email. The commit in GoogleCloudPlatform/terraform-google-conversion#98 is assigned a different email than the one used in this repo.

Do you mind adding that email to your CLA or temporarily changing your primary email setting?

The GitHub setting can be modified at https://github.com/settings/emails. If you choose to change it temporarily, I'll regenerate the downstream so that it's attributed to the appropriate author and then merge this PR. After that, you can safely revert it to your original setting.

If you're uncomfortable with either option, please let me know.

@khanali21
Copy link
Contributor Author

@rileykarson Done.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 71c3e7d.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.

New Pull Requests

I 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.
depends: GoogleCloudPlatform/terraform-google-conversion#177
depends: modular-magician/inspec-gcp#189

@rileykarson
Copy link
Member

rileykarson commented Aug 23, 2019

Hmm, that didn't seem to take. Can you confirm whether you added your email to your corp CLA or changed your GH settings? And if you changed your GH settings, can you confirm that your corporate email has the primary tag on https://github.com/settings/emails?

(the inspec PR generated is ephemeral, the Magician CI has a couple race conditions and we hit one of them.)

@khanali21
Copy link
Contributor Author

I set the primary address in github to be my corporate address.

@khanali21
Copy link
Contributor Author

@rileykarson I have squashed and merged in one commit, hopefully this will work.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 97479d3.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Inspec.

New Pull Requests

I 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.
depends: GoogleCloudPlatform/terraform-google-conversion#179
depends: modular-magician/ansible#371

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Ah, looks like it's not changing the commit downstream because the contents are the same. I'm applying a change here and then regenerating, this may resolve the issue.

products/appengine/api.yaml Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, f6a1698.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
Ansible already has an open PR.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 6049abb.

Pull request statuses

No diff detected in terraform-provider-google-beta.
No diff detected in Inspec.

New Pull Requests

I 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.
depends: GoogleCloudPlatform/terraform-google-conversion#180
depends: hashicorp/terraform-provider-google#4336
depends: modular-magician/ansible#372

Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, eea7d2f.

Pull request statuses

No diff detected in terraform-provider-google-beta.
No diff detected in Inspec.

New Pull Requests

I 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.
depends: GoogleCloudPlatform/terraform-google-conversion#181
depends: hashicorp/terraform-provider-google#4337
depends: modular-magician/ansible#373

@modular-magician modular-magician merged commit 32756c9 into GoogleCloudPlatform:master Aug 26, 2019
@rileykarson
Copy link
Member

After encountering #2244, merged manually using the Magician user.

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.

5 participants