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

Cherry-picks to support 1.20.0 release. #2567

Merged
merged 82 commits into from
Dec 13, 2018
Merged

Cherry-picks to support 1.20.0 release. #2567

merged 82 commits into from
Dec 13, 2018

Conversation

nat-henderson
Copy link
Contributor

@nat-henderson nat-henderson commented Dec 4, 2018

This is a branch made up of every cherrypick I could easily include in this release (and some that weren't super easy... but it all compiles now and I'll be running CI tests ASAP).

Here's what we should do:

  • merge this into master.
  • release 1.20.0
  • do a giant revert commit - everything since e64502a.
  • merge 2.0.0 into master.

=====
(we had an old plan - it's here for reference)

  • merge this into master
  • release 1.20.0
  • wait for tf 0.12, revendor core libraries and etc on the 2.0.0 branch.
  • shudder, grit our teeth, and git fetch; git checkout tpg/2.0.0; git branch master -f; git push tpg master -f.

This will have the effect of throwing everything since

$ git merge-base master 2.0.0
e64502a0b5e22a40a6bce850a064480f2e0694da
$ git show e64502a0b5e22a40a6bce850a064480f2e0694da
commit e64502a0b5e22a40a6bce850a064480f2e0694da
Author: tf-release-bot <[email protected]>
Date:   Fri Oct 12 18:45:52 2018 +0000

    Cleanup after v1.19.1 release

away. That is 100% correct - this history is wrong and as far as I know, everything after that commit is also correctly merged into 2.0.0. A ping @rileykarson to confirm that 8a62911 is correctly merged into 2.0.0.

The only other commits between e64502a (the point of divergence between master and 2.0.0) and 2f6b39e (master, before this PR) are two reverts (which we can safely remove) and a typo fix (77a4497) which was added accidentally.

@ghost ghost added the size/xxl label Dec 4, 2018
@paddycarver paddycarver self-requested a review December 4, 2018 19:11
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

Just a couple of questions for things that jumped out at me that I couldn't find the rationale for. Sorry if the rationale is in the commits, I had a hard time trying to track them down.

My only real concern is that these changes are included as part of an entire commit that has been reviewed, and are included on purpose. Basically, that they're not mistakes with the cherrypicking.

@@ -12,22 +12,22 @@ import (

var IamComputeSubnetworkSchema = map[string]*schema.Schema{
"subnetwork": {
Deprecated: "This resource is in beta and will be removed from this provider. Use it in the the google-beta provider instead. See https://terraform.io/docs/providers/google/provider_versions.html for more details.",
Deprecated: "This field is in beta and will be removed from this provider. Use it in the the google-beta provider instead. See https://terraform.io/docs/providers/google/provider_versions.html for more details.",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like #2271 did this, and this was the final state of the file at HEAD in 2.0.0. It was then removed (#2398). The "resource" text would be preferred here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm agreed on the resource text being preferred here, but don't know how possible that is.

google/resource_compute_firewall_test.go Show resolved Hide resolved
@nat-henderson
Copy link
Contributor Author

nat-henderson commented Dec 5, 2018

We have got a couple of fun ones here.

  • (copied from pr comment): Commit 47d4474 (the cherry-pick of 5b70a02) removes some scaffolding for tests that we didn't need over there on Earth-616, but which we do need in this timeline. I have fixed it by rebasing the branch onto itself, going back in this alternate timeline, and creating a new timeline in which that removal never happened. Someday I'll mistype a git command and be my own grandfather.

  • The stackdriver resources depend on replacevarsfortest, which was added as part of a PR that seems to introduce a breaking change, which we cannot have in this PR. Time for some selective cherry-picking! did you know git can do that? selective diff hunk adds! git: it's amazing.

  • vendor.json picked up a change to a vendored dependency but the actual change isn't here - wonder what happened there.

modular-magician and others added 24 commits December 5, 2018 18:01
<!-- This change is generated by MagicModules. -->
/cc @rileykarson
<!-- This change is generated by MagicModules. -->
/cc @danawillow
<!-- This change is generated by MagicModules. -->
/cc @rileykarson
<!-- This change is generated by MagicModules. -->
/cc @danawillow
<!-- This change is generated by MagicModules. -->
/cc @danawillow
<!-- This change is generated by MagicModules. -->
/cc @danawillow
blocking GoogleCloudPlatform/magic-modules#630

This resource won't be tested between this merging and the MM pr merging
<!-- This change is generated by MagicModules. -->
/cc @rileykarson
<!-- This change is generated by MagicModules. -->
/cc @danawillow

Fixes #2286
paddycarver
paddycarver previously approved these changes Dec 12, 2018
rileykarson
rileykarson previously approved these changes Dec 12, 2018
Copy link
Collaborator

@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.

I don't see anything egregiously wrong running through at a high level, and every code change I looked at in depth was correct.

CHANGELOG.md Outdated Show resolved Hide resolved
danawillow
danawillow previously approved these changes Dec 12, 2018
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Skimmed through, looks reasonable

CHANGELOG.md Show resolved Hide resolved
Co-Authored-By: ndmckinley <[email protected]>
@nat-henderson
Copy link
Contributor Author

What! I accepted Riley's suggestion and now it told me I've dismissed your stale approvals! Man, I'm never clicking that button again.

paddycarver
paddycarver previously approved these changes Dec 12, 2018
rileykarson
rileykarson previously approved these changes Dec 12, 2018
Copy link
Collaborator

@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.

oh that's so sad

@nat-henderson
Copy link
Contributor Author

All right that's not good. We're gonna fix that.

@nat-henderson nat-henderson merged commit 73d127a into master Dec 13, 2018
@ghost
Copy link

ghost commented Jan 12, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jan 12, 2019
@rileykarson rileykarson deleted the zany-git-stuff branch March 1, 2019 00:25
@rileykarson rileykarson restored the zany-git-stuff branch March 1, 2019 00:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.