Skip to content

Conversation

@robogatikov
Copy link
Contributor

@robogatikov robogatikov commented Sep 26, 2023


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

az aks update --network-policy=none
az aks update --network-policy=azure
az aks update --network-policy=cilium

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

live test recording file: test_aks_migrate_cluster_to_cilium_dataplane.txt

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Sep 26, 2023

⚠️Azure CLI Extensions Breaking Change Test
⚠️aks-preview
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd aks update cmd aks update added parameter network_policy

@azure-client-tools-bot-prd
Copy link

Hi @robogatikov,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Sep 26, 2023

AKS

Copy link
Contributor

Choose a reason for hiding this comment

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

in addition to the live test you added, should we have a unit test in test_managed_cluster_decorator.py? Maybe add something to test_update_network_plugin_settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test case to update network-policy from empty string to "azure"

Copy link
Member

Choose a reason for hiding this comment

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

Python implicitly converts "" to False, if user specifies --network-policy "", the code will not update the property value to "", is this expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to update the test fixtures after running the live test? I would have expected this to change the PUT MC request, but don't see any changes in recordings/test_aks_migrate_cluster_to_cilium_dataplane.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about test fixtures, probably @FumingZhang or @zhoxing-ms can help clarify

Copy link
Member

Choose a reason for hiding this comment

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

Queued live test to validate the change. If test passed, the recording file would be published as pipeline artifact, could download and commit it.

Copy link
Member

@FumingZhang FumingZhang Sep 28, 2023

Choose a reason for hiding this comment

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

The test failed due to

E azure.core.exceptions.HttpResponseError: (PropertyChangeNotAllowed) Changing property 'networkProfile.networkPolicy' is not allowed.
E Code: PropertyChangeNotAllowed
E Message: Changing property 'networkProfile.networkPolicy' is not allowed.
E Target: networkProfile.networkPolicy

To test the feature, need a feature flag or specific sub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @FumingZhang. The test failed because the change that would allow updating network policy to "cilium" has not been rolled out to eastus yet. We should wait a few days for it to reach eastus I suppose unless there is some other option.

@robogatikov robogatikov force-pushed the aks-preview/network-policy branch from 6726444 to 14cb218 Compare September 26, 2023 23:22
Copy link
Member

Choose a reason for hiding this comment

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

Queued live test to validate the change. If test passed, the recording file would be published as pipeline artifact, could download and commit it.

Copy link
Member

Choose a reason for hiding this comment

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

Python implicitly converts "" to False, if user specifies --network-policy "", the code will not update the property value to "", is this expected?

@FumingZhang
Copy link
Member

Requeued live test

Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

LGTM

live test passed

@robogatikov robogatikov changed the title {AKS} add --network-policy to az aks update {AKS} az aks update: Add --network-policy to support updating the mode of a network policy Oct 19, 2023
@zhoxing-ms
Copy link
Contributor

Please take a look at this comment #6809 (comment)

@robogatikov
Copy link
Contributor Author

Please take a look at this comment #6809 (comment)

Thanks, @zhoxing-ms , added description to HISTORY.rst and bumped extension version to 1.5.161

@yonzhan
Copy link
Collaborator

yonzhan commented Oct 20, 2023

Please fix conflicting files

@robogatikov
Copy link
Contributor Author

Please fix conflicting files

Thanks @yonzhan , forgot to sync my fork. Resolved merge conflicts

@zhoxing-ms zhoxing-ms merged commit 89d352c into Azure:main Oct 25, 2023
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ aks-preview ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=99574&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants