-
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
add terraform-provider-google-beta to ci #502
Conversation
I am a robot that works on MagicModules PRs! Once this PR is approved, you can feel free to merge it without taking any further steps. |
I am a robot that works on MagicModules PRs! Once this PR is approved, you can feel free to merge it without taking any further steps. |
I am a robot that works on MagicModules PRs! Once this PR is approved, you can feel free to merge it without taking any further steps. |
The create-prs step failed for unreleated reasons after the PR had already been opened, which I think is why the magician is confused about whether there are any downstream changes. However, it did open hashicorp/terraform-provider-google-beta#2, which looks right. This is ready for review now, but shouldn't be submitted until we catch up beta with ga, since some of these changes are things that have already been made. We also probaably need to get interconnects to generate into here, since otherwise it's going to keep trying to remove it until that PR is submitted in MM. |
.ci/magic-modules/create-pr.sh
Outdated
echo "Terraform - did not generate a PR." | ||
fi | ||
popd | ||
for VERSION in '' 'beta'; do |
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.
Do we someday plan on an tpg-alpha, or something like that, as well? Basically, I think that either we should have this for-each thing in .ci/ci.yml.tmpl
and in here, or else neither, and just duplicate the code below here. I'm inclined towards the first option, but if you think the second is better, or if you think as-written this strikes the right balance, that also makes sense to me.
@@ -0,0 +1,24 @@ | |||
--- | |||
# This file takes two inputs: magic-modules-branched in detached-HEAD state, and the list of patches. | |||
# It spits out "terraform-generated", a terraform repo on a new branch (named after the |
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.
Comment needs updating. (I wish this file wasn't necessary, ugh, seems like a big pain for no good reason, but it is since the output name changes)
|
||
# Resources that were already using beta APIs before they started being autogenerated | ||
bundle exec compiler -v beta -p products/compute -t Address,Firewall,ForwardingRule,GlobalAddress,RegionDisk,Subnetwork,VpnTunnel -e terraform -o "${GOPATH}/src/github.com/terraform-providers/terraform-provider-google/" | ||
bundle exec compiler -v beta -p products/compute -t Address,Firewall,ForwardingRule,GlobalAddress,RegionDisk,Subnetwork,VpnTunnel -e terraform -o "${GOPATH}/src/github.com/terraform-providers/$PROVIDER_NAME/" | ||
|
||
# This command can crash - if that happens, the script should not fail. | ||
set +e |
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 where you'll need to add --tag terraform-beta
if you want the magician to understand differences in pr template tags.
git submodule sync build/terraform | ||
ssh-agent bash -c "ssh-add ~/github_private_key; git submodule update --remote --init build/terraform" | ||
git add build/terraform | ||
for VERSION in '' 'beta'; do |
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 at least as complicated as just doubling up the code below - same comment as above re: duplication vs iteration.
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 0150b36) have been included in your existing downstream PRs. |
All right, this is finally ready. @ndmckinley for CI stuff (and as much tf stuff as you feel like reviewing) For the copied-over files, I find it useful to look at the downstreams alongside what's here to see what got generated. |
echo "Terraform - did not generate a PR." | ||
fi | ||
popd | ||
for VERSION in "${TERRAFORM_VERSIONS[@]}"; do |
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 still kind of think this is a sort of false generality - the best version of generality would be a lookup from version string to provider name. I also kind of think that it's odd to special-case one out of two values here.
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.
just to confirm what this would mean in practice- having a map variable that we can do the PROVIDER_NAME/SUBMODULE_DIR/etc lookups in?
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.
That's the best implementation I can imagine, yeah. As-is, I argue we're very likely to need to change it up if we ever add an tpg-alpha, and a lookup gives us flexibility.
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 great!
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!
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.
For Terraform only and not CI:
I think everything looks correct - there's no way I can be fully confident in a +- like 7500 line change but if both downstreams build & we run them through the CI gauntlet I'd be happy
'<%= dir -%>/<%= fname.split(".erb")[0] -%>': 'provider/terraform/resources/<%= fname -%>' | ||
<% end -%> | ||
<% | ||
Dir["provider/terraform/data_sources/*.go.erb"].each do |file_path| |
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.
without the prefix
logic, will this compile every file each time that ./compiler gets called. Once for each product?
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.
Yup. We talked about this a bit in the meeting today, but Emily had realized that we don't have something generate on every api prefix that we would want, so even if we do a compiler -a
, we'll still miss some things. Rather than trying to do something super complicated, we'll just compile/copy everything every time until it becomes enough of a nuisance that someone comes up with a good solution :)
@@ -46,7 +46,14 @@ resource "google_compute_instance_group_manager" "foobar" { | |||
name = "<%= ctx[:vars]['igm_name'] %>" | |||
zone = "us-central1-f" | |||
|
|||
<% if ctx[:version].nil? || ctx[:version] == 'ga' -%> |
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.
Some templates have ctx[:version].nil?
and others have if version.nil?
is there a reason for the difference?
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.
The examples/generated tests use ctx
because that's just how compile_file
(the compile command that compiles those in custom_code.rb
) works- you pass it a hash of variables that you want the file to have access to. For whatever reason, it's different from compile_files
, which is what the other stuff that gets compiled uses.
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit e12bacd) have been included in your existing downstream PRs. |
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit b0391d7) have been included in your existing downstream PRs. |
@chrisst, if you want to take another look, the downstream is looking a fair bit nicer now. |
Tracked submodules are build/puppet/_bundle build/puppet/auth build/puppet/bigquery build/puppet/compute build/puppet/sql build/puppet/storage build/puppet/spanner build/puppet/container build/puppet/dns build/puppet/pubsub build/puppet/resourcemanager build/chef/_bundle build/chef/auth build/chef/compute build/chef/sql build/chef/storage build/chef/spanner build/chef/container build/chef/dns build/chef/iam build/terraform-beta build/terraform build/ansible build/inspec.
This adds all the steps necessary to generate all the resources at their beta version into tpg-beta.
[all]
[terraform]
Output from magician generating into ga and beta providers
[terraform-beta]
[puppet]
[puppet-bigquery]
[puppet-compute]
[puppet-container]
[puppet-dns]
[puppet-logging]
[puppet-pubsub]
[puppet-resourcemanager]
[puppet-sql]
[puppet-storage]
[chef]
[chef-compute]
[chef-container]
[chef-dns]
[chef-logging]
[chef-spanner]
[chef-sql]
[chef-storage]
[ansible]