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

azurerm_kubernetes_cluster - making linux_profile optional #1821

Merged
merged 10 commits into from
Aug 30, 2018

Conversation

metacpp
Copy link
Contributor

@metacpp metacpp commented Aug 24, 2018

This PR is going to resolve the #1806.

The latest AKS API does not require linux_profile to be set, and Azure portal follows it. Terraform still requires it by default, this inconsistency caused the importing issue.

Manually created instance on Azure portal, and did terraform import and terraform plan, no change is required.

The AKS API does not require linux_profile to be set by default, this is enabled by default on Azure
portal. Terraform's schema does not align with this, which causes importing issue.
Pass empty string to refactored function in import test of AKS.
@ghost ghost added the size/M label Aug 24, 2018
Added the linux_profile configuration string while baking the configuration.
@metacpp
Copy link
Contributor Author

metacpp commented Aug 24, 2018

@zqingqing1 feel free to review it.

@metacpp metacpp changed the title Make linux_profile be optional linux_profile should be optional to align with API spec Aug 24, 2018
@metacpp metacpp requested review from WodansSon and katbyte August 24, 2018 01:51
@zqingqing1
Copy link

zqingqing1 commented Aug 24, 2018

lgtm

Remove the basic test with linux profile to avoid string concat issue.
@ghost ghost added the size/M label Aug 24, 2018
@metacpp metacpp self-assigned this Aug 24, 2018
@metacpp metacpp added this to the 1.14.0 milestone Aug 24, 2018
linux_profile needs to be computed since it's optional. Otherwise, return `terraform plan` will warn
with change.
@ghost ghost added the size/M label Aug 24, 2018
If the response of field is not returned in response body, need to return empty array.
@ghost ghost added the size/M label Aug 25, 2018
@metacpp
Copy link
Contributor Author

metacpp commented Aug 25, 2018

Automation test result:
screen shot 2018-08-24 at 8 25 01 pm

@tombuildsstuff tombuildsstuff removed the request for review from katbyte August 28, 2018 18:15
Copy link
Contributor

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

We need to add a test to cover the existing case - but this otherwise LGTM 👍

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

LGTM once the TestAccAzureRMKubernetesCluster_linuxProfile and import test is added.

Add 2 tests to have more coverage on linuxProfile specific scenarios.
@ghost ghost added size/L and removed size/M labels Aug 29, 2018
Removed useless linux_profile and kubernetes_version settings for tests.
Add newline to import test file for AKS.
Fix the path issue to correct naming for linux_profile.
@metacpp
Copy link
Contributor Author

metacpp commented Aug 30, 2018

Reran all 13 tests for AKS resource:
screen shot 2018-08-29 at 9 21 45 pm

Copy link
Contributor

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

@tombuildsstuff tombuildsstuff changed the title linux_profile should be optional to align with API spec azurerm_kubernetes_cluster - making linux_profile optional Aug 30, 2018
@tombuildsstuff tombuildsstuff merged commit 244f1d7 into master Aug 30, 2018
@tombuildsstuff tombuildsstuff deleted the linux_profile_fix branch August 30, 2018 12:43
tombuildsstuff added a commit that referenced this pull request Aug 30, 2018
torresdal pushed a commit to torresdal/terraform-provider-azurerm that referenced this pull request Aug 31, 2018
`azurerm_kubernetes_cluster` - making `linux_profile` optional
torresdal pushed a commit to torresdal/terraform-provider-azurerm that referenced this pull request Aug 31, 2018
jbcom pushed a commit to platonic-io/terraform-provider-azurerm that referenced this pull request Sep 27, 2018
* timcurless/timcurless:
  Fixing issues with aadProfile server_app_secret always causing a new cluster
  Adding documentation updates
  Updating to include hashicorp#1845
  Authentication: registering all clients consistently (hashicorp#1845)
  Updating to include hashicorp#1843
  r/Logic App: ensuring parameters are strings prior to setting (hashicorp#1843)
  Updating to include hashicorp#1821
  linux_profile should be optional to align with API spec (hashicorp#1821)
  Updating to include hashicorp#1816
  Storage: Import Support (hashicorp#1816)
  Updating to include hashicorp#1835
  Allow azurerm_function_app to use upper case names in consumption plan (hashicorp#1835)
  formatting
@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 👉 [email protected]. Thanks!

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

4 participants