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

Update connectedk8s helm binary download source & introduce new --skip-cluster-ssl-verification flag #7294

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

sidiesen
Copy link
Contributor


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

Related command

az connectedk8s

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? (pip install wheel==0.30.0 required)
  • [ x ] My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

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.

Copy link

azure-client-tools-bot-prd bot commented Feb 17, 2024

⚠️Azure CLI Extensions Breaking Change Test
⚠️connectedk8s
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd connectedk8s connect cmd connectedk8s connect added parameter skip_ssl_verification
⚠️ 1006 - ParaAdd connectedk8s delete cmd connectedk8s delete added parameter skip_ssl_verification
⚠️ 1006 - ParaAdd connectedk8s disable-features cmd connectedk8s disable-features added parameter skip_ssl_verification
⚠️ 1006 - ParaAdd connectedk8s enable-features cmd connectedk8s enable-features added parameter skip_ssl_verification
⚠️ 1006 - ParaAdd connectedk8s troubleshoot cmd connectedk8s troubleshoot added parameter skip_ssl_verification
⚠️ 1006 - ParaAdd connectedk8s update cmd connectedk8s update added parameter skip_ssl_verification
⚠️ 1006 - ParaAdd connectedk8s upgrade cmd connectedk8s upgrade added parameter skip_ssl_verification

@yonzhan
Copy link
Collaborator

yonzhan commented Feb 17, 2024

Thank you for your contribution! We will review the pull request and get back to you soon.

@sidiesen sidiesen force-pushed the sidiesen/helm_location branch 2 times, most recently from b9b4c63 to 86239eb Compare February 20, 2024 23:35
@yonzhan
Copy link
Collaborator

yonzhan commented Feb 21, 2024

Please fix CI issues.

@sidiesen sidiesen force-pushed the sidiesen/helm_location branch 2 times, most recently from 7b0e248 to 3dcd47a Compare February 21, 2024 01:36
@sidiesen sidiesen force-pushed the sidiesen/helm_location branch 4 times, most recently from 3dcd47a to f4deaa5 Compare February 22, 2024 22:03
@sidiesen sidiesen force-pushed the sidiesen/helm_location branch 3 times, most recently from 911f53f to 0ff2c3b Compare February 27, 2024 19:04
Copy link

⚠️ Release Suggestions

Module: connectedk8s

  • Update version to 1.7.0 in setup.py

Notes

  • Stable/preview tag is inherited from last release. If needed, please add stable/preview label to modify it.
  • Major/minor/patch/pre increment of version number is calculated by pull request code changes automatically. If needed, please add major/minor/patch/pre label to adjust it.
  • For more info about extension versioning, please refer to Extension version schema

src/connectedk8s/HISTORY.rst Outdated Show resolved Hide resolved

@live_only()
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why do you need to mark this test as @live_only()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is exercising the exact same paths as a different test in this file that is already marked as @live_only(), just with the added parameter to skip ssl verification when connecting to the cluster.
I believe that test was marked as live only as the necessary creation of Azure resources in these tests was causing issues in non-live test settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks~

@@ -49,6 +49,7 @@ def load_arguments(self, _):
c.argument('no_wait', options_list=['--no-wait'], arg_group='Timeout', help="Do not wait for the long-running operation to finish.")
c.argument('correlation_id', options_list=['--correlation-id'], help='A guid that is used to internally track the source of cluster onboarding. Please do not modify it unless advised', validator=override_client_request_id_header)
c.argument('container_log_path', help='Override the default container log path to enable fluent-bit logging')
c.argument('skip_ssl_verification', options_list=['--skip-ssl-verification'], action='store_true', help='Skip SSL verification for any cluster connection.')
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, the default option name does not need to be defined repeatedly

Suggested change
c.argument('skip_ssl_verification', options_list=['--skip-ssl-verification'], action='store_true', help='Skip SSL verification for any cluster connection.')
c.argument('skip_ssl_verification', action='store_true', help='Skip SSL verification for any cluster connection.')

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 for the pointer, I updated the argument definitions.

@zhoxing-ms zhoxing-ms merged commit 487d71d into Azure:main Apr 19, 2024
14 of 15 checks passed
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ connectedk8s ] : https://dev.azure.com/azclitools/release/_build/results?buildId=150393&view=results

blackchoey pushed a commit to blackchoey/azure-cli-extensions that referenced this pull request Apr 29, 2024
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.

6 participants