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

aks: vendor 20201201 sdk and add 'enable-encryption-at-host' parameter #2912

Merged
merged 3 commits into from
Jan 25, 2021

Conversation

andyzhangx
Copy link
Contributor

@andyzhangx andyzhangx commented Jan 18, 2021


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

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

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@yungezz
Copy link
Member

yungezz commented Jan 19, 2021

hi @andyzhangx the change looks good. Pls run live test to make sure no regression caused by API version bump. thanks.

@andyzhangx andyzhangx changed the title aks: vendor 20201201 sdk aks: vendor 20201201 sdk and add 'enable-encryption-at-host' support Jan 19, 2021
@andyzhangx andyzhangx changed the title aks: vendor 20201201 sdk and add 'enable-encryption-at-host' support aks: vendor 20201201 sdk and add 'enable-encryption-at-host' parameter Jan 19, 2021
fix recording test failures

change aks default version to 20201201

fix test failures

fix test case

fix image names
aks: add history

aks: fix go linter

aks: fix linter
@andyzhangx
Copy link
Contributor Author

hi @andyzhangx the change looks good. Pls run live test to make sure no regression caused by API version bump. thanks.

@yungezz I have run the live tests and also fixed the test case issues, PTAL again, hanks.

@andyzhangx
Copy link
Contributor Author

@yungezz could you merge this PR? thanks.

@@ -123,6 +123,7 @@ def load_arguments(self, _):
c.argument('appgw_subnet_id', options_list=['--appgw-subnet-id'], arg_group='Application Gateway')
c.argument('appgw_watch_namespace', options_list=['--appgw-watch-namespace'], arg_group='Application Gateway')
c.argument('aci_subnet_name', type=str)
c.argument('enable_encryption_at_host', options_list=['--enable-encryption-at-host'], action='store_true')
Copy link
Member

Choose a reason for hiding this comment

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

Is this a property that will be persisted in resource? If so, then get_three_state_flag() is recommended instead of using action='store_true'

Copy link
Contributor Author

@andyzhangx andyzhangx Jan 20, 2021

Choose a reason for hiding this comment

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

hi @fengzhou-msft the property is a bool type which is persisted in resource.
what's the diff between store_true and get_three_state_flag? I found all AKS code are using store_true:

        c.argument('enable_cluster_autoscaler', options_list=["--enable-cluster-autoscaler", "-e"], action='store_true')
        c.argument('disable_cluster_autoscaler', options_list=["--disable-cluster-autoscaler", "-d"], action='store_true')
        c.argument('update_cluster_autoscaler', options_list=["--update-cluster-autoscaler", "-u"], action='store_true')

Do you mean I should change to following? Thanks.

c.argument('enable_encryption_at_host', arg_type=get_three_state_flag(), help='Enable EncryptionAtHost.'

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, fixed.

@yungezz
Copy link
Member

yungezz commented Jan 25, 2021

hi @fengzhou-msft is this PR ready to merge? thanks

@Azure Azure deleted a comment from yonzhan Jan 25, 2021
@fengzhou-msft fengzhou-msft merged commit d0751c6 into Azure:master Jan 25, 2021
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.

5 participants