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

Upgrade ContainerServices 2017-09-30 to 2018-03-31 #1474

Merged
merged 6 commits into from Jul 2, 2018

Conversation

lfshr
Copy link
Contributor

@lfshr lfshr commented Jul 2, 2018

Upgraded azurerm containerservices from 2017-09-30 to 2018-03-31.
This is an upgrade only and does not contain any new features introduced in 2018-03-31.

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for this 👍

@lfshr
Copy link
Contributor Author

lfshr commented Jul 2, 2018

@tombuildsstuff no problem. I'll probably take something very minor in the advanced networking stuff to work on if you guys are agreeable. Wanting to dip my toes in the water without taking too much on 😏

@tombuildsstuff
Copy link
Member

@lfshr

I'll probably take something very minor in the advanced networking stuff to work on if you guys are agreeable. Wanting to dip my toes in the water without taking too much on 😏

Within AKS, or another resource? That's sounds good from our end 😄 - I'd suggest commenting on an existing issue to claim it/creating a new one for it if it doesn't exist (we can assign you to it too). One word of warning specifically for the Networking API's is that the older ones can have bugs in the API around resource deletion, but I'd suggest looking into it either way :)

@lfshr
Copy link
Contributor Author

lfshr commented Jul 2, 2018

@tombuildsstuff for now I'm looking at AKS and Google Cloud Platform. EKS is on the cards as well. ATM this has been mostly driven by the business requirements of my current employer, but I'd be open to smaller tasks being assigned to me. I've literally just started working with GO on this PR so it really does have to be "dip your toes" level issues that I work on. (Also it's very very sunny in Scotland right now which never happens so my time is being spent sunning it up) 😆

@bergerx
Copy link

bergerx commented Jul 2, 2018

This is an upgrade only and does not contain any new features introduced in 2018-03-31.

Is this correct? I'm seeing fields for many recent AKS features are added in this patch ( https://github.com/terraform-providers/terraform-provider-azurerm/pull/1474/files#diff-2806273e153e90addd33508a3778e6ffR1198 ), so I guess after this is merged we'll be able to use those new features introduced when AKS went GA which were not available in 2017-09-30.

Or is there still some other steps to be taken before starting use those features?

@lfshr
Copy link
Contributor Author

lfshr commented Jul 2, 2018

@bergerx that is correct. The only thing that has been upgraded is the vendor, so what you're seeing is Microsoft's new implementations. Further steps have to be taken to add these features into the Terraform resource itself.
You can find the source of the new vendor here.

@tombuildsstuff tombuildsstuff added this to the 1.9.0 milestone Jul 2, 2018
@lfshr
Copy link
Contributor Author

lfshr commented Jul 2, 2018

@tombuildsstuff I can't merge this 😢

@tombuildsstuff
Copy link
Member

@lfshr I'm running the tests atm, they detected one issue I'm pushing a fix for (turns out AKS uses the same CIDR block we did, that conflicts) - but it otherwise looks good so far :)

@tombuildsstuff
Copy link
Member

Data Source tests pass, apart from a conflict with the AKS Subnet CIDR range which is fixed in b1d6a21:

screenshot 2018-07-02 at 15 13 15

@tombuildsstuff
Copy link
Member

ACS Tests pass:

screenshot 2018-07-02 at 15 14 33

@tombuildsstuff
Copy link
Member

confirmed fixed in b1d6a21:

$ acctests azurerm TestAccAzureRMKubernetesCluster_internalNetwork
=== RUN   TestAccAzureRMKubernetesCluster_internalNetwork
--- PASS: TestAccAzureRMKubernetesCluster_internalNetwork (1245.15s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	1245.201s

(this is the resource test, but they both run the same configuration)

@tombuildsstuff
Copy link
Member

tombuildsstuff commented Jul 2, 2018

AKS Resource tests pass (excl. the known test above fixed in commit b1d6a21)

screenshot 2018-07-02 at 15 28 37

@tombuildsstuff
Copy link
Member

@lfshr LGTM - thanks for this 👍

@tombuildsstuff tombuildsstuff merged commit 003c125 into hashicorp:master Jul 2, 2018
tombuildsstuff added a commit that referenced this pull request Jul 2, 2018
tombuildsstuff added a commit that referenced this pull request Jul 2, 2018
@lfshr lfshr deleted the containerservices/2018-03-31 branch July 2, 2018 13:39
@ghost
Copy link

ghost commented Mar 30, 2020

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants