Skip to content

Managed cassandra preview commands for cli extension#3948

Merged
evelyn-ys merged 28 commits intoAzure:mainfrom
vivek-microsoft:users/visunda/preview-managedCassandra
Dec 15, 2021
Merged

Managed cassandra preview commands for cli extension#3948
evelyn-ys merged 28 commits intoAzure:mainfrom
vivek-microsoft:users/visunda/preview-managedCassandra

Conversation

@vivek-microsoft
Copy link
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.

@yonzhan
Copy link
Collaborator

yonzhan commented Oct 11, 2021

cosmosdb

Comment on lines 39 to 42
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these commands since it will be supported in azure-cli repo.

@evelyn-ys
Copy link
Member

Please add tests for new commands

@evelyn-ys
Copy link
Member

evelyn-ys commented Oct 21, 2021

Since the api version has upgraded, I think you might need to re-record all the existing tests. Run azdev test cosmosdb-preview --live to re-record

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean cassandra_stop_start by cassandra-stop-start?

@jiasli
Copy link
Member

jiasli commented Oct 22, 2021

Please sync from main to avoid pipeline being run for other extensions.

Copy link
Member

Choose a reason for hiding this comment

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

There's no backups command, only backup command group and backup list/backup show command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this if condition is not met, data_center_properties.authentication_method_ldap_properties is not set at all, but still the serialized payload contains data_center_properties.authentication_method_ldap_properties as {}

Instead I expect authentication_method_ldap_properties key not to be part of the payload itself.

Comment on lines 39 to 42
Copy link
Member

Choose a reason for hiding this comment

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

These commands have already been in main CLI, why add them in extension again?

Copy link
Member

Choose a reason for hiding this comment

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

Generally we will design CLI commands like managed0cassandra cluster backup list

Copy link
Member

Choose a reason for hiding this comment

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

Same here managed-cassandra cluster backup show

Copy link
Member

@evelyn-ys evelyn-ys Dec 9, 2021

Choose a reason for hiding this comment

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

Suggested change
'managed-cassandra cluster backup-list']:
'managed-cassandra cluster backup-list',
'managed-cassandra cluster backup-show']:

Does backup-show share the same definition of cluster_name param? If so, you should add backup-show in the list too.

Comment on lines 37 to 38
Copy link
Member

@evelyn-ys evelyn-ys Dec 9, 2021

Choose a reason for hiding this comment

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

Suggested change
g.custom_command('backup-list', 'cli_cosmosdb_managed_cassandra_cluster_list_backup', is_preview=True)
g.custom_command('backup-show', 'cli_cosmosdb_managed_cassandra_cluster_show_backup', is_preview=True)
g.custom_command('backup list', 'cli_cosmosdb_managed_cassandra_cluster_list_backup', is_preview=True)
g.custom_command('backup show', 'cli_cosmosdb_managed_cassandra_cluster_show_backup', is_preview=True)

Better to change them to backup list and backup show like all other commands.

@vivek-microsoft vivek-microsoft force-pushed the users/visunda/preview-managedCassandra branch from dcd0c46 to f701de0 Compare December 10, 2021 06:33
@evelyn-ys evelyn-ys merged commit 6f71dee into Azure:main Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants