-
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
AppEngine Standard Application Version resource. #1884
AppEngine Standard Application Version resource. #1884
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. |
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. |
Is this conflict build/terraform-beta expected? |
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.
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
.
templates/terraform/examples/app_engine_standard_app_version.tf.erb
Outdated
Show resolved
Hide resolved
Hi, |
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 |
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 |
Oops, sorry for the extra round trip- can you do the same for |
No my fault should have checked the api.yml for any other resource. Many thanks for your support @rileykarson |
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.
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
- script
- 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" |
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.
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.
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 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..
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.
Generated downstreams and attempted to run the generated test, I noticed a syntax error in it.
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. |
@rileykarson Hi Riley, Is there anything I need to do? Please let me know. |
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. ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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!
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.
@rileykarson Done. |
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 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. |
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 (the inspec PR generated is ephemeral, the Magician CI has a couple race conditions and we hit one of them.) |
I set the primary address in github to be my corporate address. |
@rileykarson I have squashed and merged in one commit, hopefully this will work. |
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 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.
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.
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. |
f6a1698
to
6049abb
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-provider-google-beta. 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. |
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-provider-google-beta. 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. |
After encountering #2244, merged manually using the Magician user. |
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)