Skip to content

aks-preview: support csi drivers extensibility#4742

Merged
zhoxing-ms merged 1 commit into
Azure:mainfrom
ZeroMagic:jiliu8/disable-csi
May 7, 2022
Merged

aks-preview: support csi drivers extensibility#4742
zhoxing-ms merged 1 commit into
Azure:mainfrom
ZeroMagic:jiliu8/disable-csi

Conversation

@ZeroMagic
Copy link
Copy Markdown
Contributor


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.

@ghost ghost requested review from wangzelin007 and zhoxing-ms April 26, 2022 17:32
@ghost ghost assigned zhoxing-ms Apr 26, 2022
@ghost ghost added this to the May 2022 (2022-05-24) milestone Apr 26, 2022
@ghost ghost added the Auto-Assign Auto assign by bot label Apr 26, 2022
@ghost ghost requested a review from yonzhan April 26, 2022 17:33
@ghost ghost added the AKS label Apr 26, 2022
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Apr 26, 2022

aks-preview

@ZeroMagic ZeroMagic force-pushed the jiliu8/disable-csi branch 6 times, most recently from 07bf84a to dae4bbb Compare April 28, 2022 19:08
Comment thread src/aks-preview/azext_aks_preview/tests/latest/test_decorator.py Outdated
Comment thread src/aks-preview/azext_aks_preview/tests/latest/test_decorator.py Outdated
@ZeroMagic ZeroMagic force-pushed the jiliu8/disable-csi branch 5 times, most recently from 16df791 to fdc879a Compare May 5, 2022 08:36
Copy link
Copy Markdown
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

Comment on lines 330 to 335
Copy link
Copy Markdown
Contributor

@zhoxing-ms zhoxing-ms May 5, 2022

Choose a reason for hiding this comment

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

We do not recommend adding two completely opposite parameters repeatedly, because --enable-disk-driver True can replace --disable-disk-driver False, it will lead to too many similar parameters of CLI which will increase the learning cost of users. Moreover, it may also lead to conflicting usage of parameters

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@zhoxing-ms so what's the suggestion? only use --enable-disk-driver? and --enable-disk-driver False looks weird.

Copy link
Copy Markdown
Contributor

@andyzhangx andyzhangx May 5, 2022

Choose a reason for hiding this comment

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

there are a few parameters following the same way in AKS: e.g.

        c.argument('enable_public_fqdn', action='store_true')
        c.argument('disable_public_fqdn', action='store_true')
        c.argument('enable_ahub')
        c.argument('disable_ahub')
        c.argument('enable_azure_rbac', action='store_true')
        c.argument('disable_azure_rbac', action='store_true')
        c.argument('enable_secret_rotation', action='store_true')
        c.argument('disable_secret_rotation', action='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('enable_pod_identity', action='store_true')
        c.argument('disable_pod_identity', action='store_true')
        c.argument('enable_workload_identity', arg_type=get_three_state_flag(), is_preview=True)
        c.argument('disable_workload_identity', arg_type=get_three_state_flag(), is_preview=True)

Signed-off-by: Ji An Liu <jiliu8@microsoft.com>
@ZeroMagic ZeroMagic force-pushed the jiliu8/disable-csi branch from fdc879a to 3839f2a Compare May 6, 2022 05:41
@ZeroMagic
Copy link
Copy Markdown
Contributor Author

@zhoxing-ms @FumingZhang
Enhance the live test. PTAL again.

@FumingZhang
Copy link
Copy Markdown
Member

Queued a pipeline to test the newly added options, works like a charm!

@zhoxing-ms zhoxing-ms merged commit 5b3b5bb into Azure:main May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AKS Auto-Assign Auto assign by bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants