Skip to content

add support for setting only_critical_addons_enabled#129

Merged
lonegunmanb merged 1 commit intoAzure:masterfrom
wondersd:support-for-only-critical-addons-enabled
Jul 8, 2022
Merged

add support for setting only_critical_addons_enabled#129
lonegunmanb merged 1 commit intoAzure:masterfrom
wondersd:support-for-only-critical-addons-enabled

Conversation

@wondersd
Copy link
Copy Markdown
Contributor

@wondersd wondersd commented Oct 29, 2021 •
edited
Loading

Changes proposed in the pull request:

Allows for only_critical_addons_enabled to be enabled for the aks clusters default node pool.

@ghost
Copy link
Copy Markdown

ghost commented Oct 29, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@adminhhanes
Copy link
Copy Markdown

adminhhanes commented Nov 30, 2021

Can someone merge this??

@paytience
Copy link
Copy Markdown

paytience commented Dec 1, 2021

Would be great if this was merged

@adminhhanes
Copy link
Copy Markdown

adminhhanes commented Dec 1, 2021

@yupwei68 could you please merge and release this PR?

@wondersd
Copy link
Copy Markdown
Contributor Author

wondersd commented Jan 25, 2022

@adminhhanes @yupwei68 any updates on when this might be merged? anything i need to do for it?

@wondersd
Copy link
Copy Markdown
Contributor Author

wondersd commented Apr 7, 2022

@lonegunmanb @jiaweitao001

Any chance I could get some eyes on this? It should be backwards compatible and would be really useful, thanks!

@lonegunmanb
Copy link
Copy Markdown
Member

lonegunmanb commented Apr 12, 2022

@lonegunmanb @jiaweitao001

Any chance I could get some eyes on this? It should be backwards compatible and would be really useful, thanks!

Sure thing, would you please change the description a little to meet HashiCorp's tradition? Others LGTM!

@wondersd wondersd changed the title add support for setting only_critical_addons_enabled on the default n… add support for setting only_critical_addons_enabled Jun 21, 2022
@wondersd wondersd force-pushed the support-for-only-critical-addons-enabled branch 2 times, most recently from bc114bb to 4f4efcd Compare June 21, 2022 16:04
@wondersd
Copy link
Copy Markdown
Contributor Author

wondersd commented Jun 21, 2022

@lonegunmanb updated description to match, https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster#only_critical_addons_enabled

@wondersd wondersd force-pushed the support-for-only-critical-addons-enabled branch 2 times, most recently from 22eb772 to f9e54c7 Compare June 21, 2022 16:11
@lonegunmanb
Copy link
Copy Markdown
Member

lonegunmanb commented Jun 22, 2022

Hi @wondersd sorry my mistake for the confusion. The actual terraform azure resource document is in the provider's code, like the following:

only_critical_addons_enabled - (Optional) Enabling this option will taint default node pool with CriticalAddonsOnly=true:NoSchedule taint. Changing this forces a new resource to be created.

As we can see the CriticalAddonsOnly=true:NoSchedule is quoted with "`". Would you please quote it as the document markdown code in the provider? Thanks.

lonegunmanb
lonegunmanb requested changes Jun 22, 2022
View reviewed changes
Comment thread variables.tf Outdated
Comment thread variables.tf Outdated
@wondersd wondersd force-pushed the support-for-only-critical-addons-enabled branch from f9e54c7 to 1cfede7 Compare June 25, 2022 22:39
@lonegunmanb
Copy link
Copy Markdown
Member

lonegunmanb commented Jun 27, 2022

Hi @wondersd thanks for updating! The pr seems to have a conflict with the current branch, would you please resolve it so I can merge this pr? Thanks!

@wondersd wondersd force-pushed the support-for-only-critical-addons-enabled branch from 1cfede7 to 83b78ec Compare June 30, 2022 15:14
@wondersd
Copy link
Copy Markdown
Contributor Author

wondersd commented Jun 30, 2022

@lonegunmanb rebased, should be all set. Thanks!

the-technat
the-technat approved these changes Jul 6, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@the-technat the-technat left a comment

Choose a reason for hiding this comment

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

LGTM, can we please merge this?

@mkilchhofer
Copy link
Copy Markdown
Contributor

mkilchhofer commented Jul 6, 2022

This is a blocker for us at Swiss Post since we need Cilium installed:

Screenshot of Cilium installation guide
Source: https://docs.cilium.io/en/v1.11/gettingstarted/k8s-install-helm/

@wondersd wondersd requested a review from lonegunmanb July 7, 2022 21:24
lonegunmanb
lonegunmanb approved these changes Jul 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@lonegunmanb lonegunmanb 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 @wondersd !

@lonegunmanb lonegunmanb merged commit 4181330 into Azure:master Jul 8, 2022
@lonegunmanb lonegunmanb mentioned this pull request Sep 29, 2022
Closed
@wondersd wondersd deleted the support-for-only-critical-addons-enabled branch November 3, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lonegunmanb lonegunmanb lonegunmanb approved these changes

+3 more reviewers

@mkilchhofer mkilchhofer mkilchhofer approved these changes

@the-technat the-technat the-technat approved these changes

@adminhhanes adminhhanes adminhhanes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wondersd @adminhhanes @paytience @lonegunmanb @mkilchhofer @the-technat
⚡