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

Added support for Windows 2022 nodes #1377

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

xrow
Copy link
Contributor

@xrow xrow commented Dec 14, 2023

SUMMARY

The feature adds the capability to create Windows 2022 nodes via the os_sku property.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_aks

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Dec 25, 2023
Copy link
Collaborator

@Fred-sun Fred-sun left a comment

Choose a reason for hiding this comment

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

small change request!

plugins/modules/azure_rm_aksagentpool.py Show resolved Hide resolved
plugins/modules/azure_rm_aks.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_aks.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_aksagentpool.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_aks.py Outdated Show resolved Hide resolved
@Fred-sun Fred-sun added the question Further information is requested label Jan 3, 2024
xrow and others added 3 commits January 3, 2024 10:49
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
@Fred-sun
Copy link
Collaborator

Fred-sun commented Jan 3, 2024

@xrow What is your version of azure-mgmt-containerservice?

@xrow
Copy link
Contributor Author

xrow commented Jan 5, 2024

@Fred-sun I believe it was using latest, but i acutally do not know. I do not know how to test.

@Fred-sun
Copy link
Collaborator

Fred-sun commented Jan 8, 2024

@xrow I am currently using the version of "azure-mgmt-containerservice==20.0.0", I will upgrade to the latest version test, thank you!

@xrow
Copy link
Contributor Author

xrow commented Jan 8, 2024

@Fred-sun Based on your feedback I added AzureLinux as a sku too. I tought that is the most reasonable todo.

@@ -343,16 +343,6 @@
serviceName=dict(
type='str',
required=True,
choices=['Microsoft.Web/serverFarms', 'Microsoft.ContainerInstance/containerGroups', 'Microsoft.Netapp/volumes',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why delete the choices? Please keep!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fred-sun I am sorry. That is actually part of a different pull request I am planning. Let me figure out on how to have multipole pulls at the same time. Can you mean comment of the remaining open topics? I will revert this part later. Sorry again, I am no so strong with pull requests and its management.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fred-sun Hi, I moved the change into a different pull #1414 .. We can go ahead on this one.

@Fred-sun
Copy link
Collaborator

@xrow Other os_sku options will only be supported in versions higher than azure-mgmt-containerservice=20.0.0. I will upgrade azure-mgmt-containerservice first, and then push forward the PR merge in testing. Thank you!

@xrow
Copy link
Contributor Author

xrow commented Jan 23, 2024

Thank you too...

@Fred-sun
Copy link
Collaborator

@xrow This branch has conflicts, Please help to resolved. The relate package has been upgrade. when you resolved, I will review and push for merged as soon as possible! Thank you very much!

@xrow
Copy link
Contributor Author

xrow commented Aug 21, 2024

@Fred-sun can you review again?

@@ -615,6 +623,12 @@
type: str
returned: always
sample: Linux
os_sku:
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplication of key "os_sku" in mapping ( line 717)

@@ -64,6 +64,14 @@
choices:
- Linux
- Windows
os_sku:
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplication of key "os_sku" in mapping( line 127)

@@ -808,6 +822,10 @@ def __init__(self):
type='str',
choices=['Linux', 'Windows']
),
os_sku=dict(
Copy link
Collaborator

Choose a reason for hiding this comment

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

duplication of key "os_sku" in mapping(line 861)

@Fred-sun
Copy link
Collaborator

kindly ping!

@xrow
Copy link
Contributor Author

xrow commented Aug 30, 2024

@Fred-sun Hi, Fred I am sorry I am not allowed to dedicate time on this any more by the organisation I am working for. I guess you have to take it from here or close it. I am sorry for it.

@Fred-sun
Copy link
Collaborator

@xrow Thank you for your feedback, you can authorize me to maintain this PR together, thank you!

@xrow
Copy link
Contributor Author

xrow commented Aug 30, 2024

@Fred-sun Please do so! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority question Further information is requested work in In trying to solve, or in working with contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants