[RDBMS] MySQL/Postgres command support by Suna-MS · Pull Request #3236 · Azure/azure-cli

Suna-MS · 2017-05-08T10:18:29Z

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

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test. (PR [RDBMS] Add Test Scenarios #3240)

msftclas · 2017-05-08T10:18:33Z

@Suna-MS,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

troydai · 2017-05-08T15:58:37Z

src/command_modules/azure-cli-rdbms/azure/cli/command_modules/rdbms/__init__.py

+
+import azure.cli.command_modules.rdbms.help  # pylint: disable=unused-import
+
+__all__ = ['load_params', 'load_commands']


The CLI itself doesn't rely on the symbols exposed in __all__ to find these two function. So to us, it is optional.

troydai · 2017-05-08T16:25:58Z

src/command_modules/azure-cli-rdbms/tests/test_rdbms_commands.py

+    def _test_db_mgmt(self, resource_group, server, engine):
+        # database_name = "cliautomationdb01"
+        # charset = 'utf8'
+        # collation = 'utf8_general_ci' if engine == 'mysql' else 'English_United States.1252'


Please remove the commented codes.

Want to keep them here because it was initially required, but to keep consistent with portal, the commands was temporarily removed, in case we need to re-open these commands later.

Then please keep the code in a staging area (git stash or commit to a separate branch). We should not archive code in the main code base.

troydai · 2017-05-08T16:26:06Z

src/command_modules/azure-cli-rdbms/tests/test_rdbms_commands.py

+        #         checks=[
+        #             JMESPathCheck('resourceGroup', rg),
+        #             JMESPathCheck('name', database_name),
+        #             JMESPathCheck('collation', collation)])


Same, please remove.

troydai · 2017-05-08T16:26:10Z

src/command_modules/azure-cli-rdbms/tests/test_rdbms_commands.py

+
+        # self.cmd('{} db delete -g {} -s {} -n {} --yes'
+        #         .format(engine, rg, server, database_name),
+        #         checks=[NoneCheck()])


Same, please remove.

troydai · 2017-05-08T16:28:06Z

src/command_modules/azure-cli-rdbms/tests/test_rdbms_commands.py

+                 checks=[NoneCheck()])
+
+        import os
+        os.remove(file_name)


This will not guarantee the file is deleted in the case when the previous assertion failed. Please use self.addCleanup to register clean up code. https://docs.python.org/2/library/unittest.html#unittest.TestCase.addCleanup

troydai · 2017-05-08T16:32:06Z

src/command_modules/azure-cli-rdbms/azure/cli/command_modules/rdbms/help.py

+
+
+add_helps("mysql", "MySQL")
+add_helps("postgres", "PostgreSQL")


I advise against using a loop in help.py file. The file is expected to be a fast access for documentation. Therefore a flat and clean format for faster accessibility to the command document trumps the need of reusability. Please flatten the loop.

We plan to split the command module to two later, i will keep it as it now, and after split to two command module, there will be no such issue. Same as below.

troydai · 2017-05-08T16:33:08Z

src/command_modules/azure-cli-rdbms/azure/cli/command_modules/rdbms/commands.py

+        'azure.mgmt.rdbms.{}.operations.log_files_operations'.format(server_type),
+        'LogFilesOperations')
+
+    def log_file_factory(args):


the factory can be defined in the context scope for a better code organizing.

codecov-io · 2017-05-08T18:04:58Z

Codecov Report

Merging #3236 into master will decrease coverage by 0.07%.
The diff coverage is 63.41%.

@@            Coverage Diff             @@
##           master    #3236      +/-   ##
==========================================
- Coverage   70.35%   70.27%   -0.08%     
==========================================
  Files         381      391      +10     
  Lines       24777    25067     +290     
  Branches     3785     3811      +26     
==========================================
+ Hits        17431    17616     +185     
- Misses       6237     6342     +105     
  Partials     1109     1109

Impacted Files	Coverage Δ
...li-rdbms/azure/cli/command_modules/rdbms/params.py	`100% <100%> (ø)`
...-cli-rdbms/azure/cli/command_modules/rdbms/help.py	`100% <100%> (ø)`
.../command_modules/azure-cli-rdbms/azure/__init__.py	`100% <100%> (ø)`
...-rdbms/azure/cli/command_modules/rdbms/__init__.py	`100% <100%> (ø)`
...mand_modules/azure-cli-rdbms/azure/cli/__init__.py	`100% <100%> (ø)`
...re-cli-rdbms/azure/cli/command_modules/__init__.py	`100% <100%> (ø)`
...li-rdbms/azure/cli/command_modules/rdbms/custom.py	`11.47% <11.47%> (ø)`
...dbms/azure/cli/command_modules/rdbms/validators.py	`29.03% <29.03%> (ø)`
...cli-rdbms/azure/cli/command_modules/rdbms/_util.py	`48.93% <48.93%> (ø)`
...-rdbms/azure/cli/command_modules/rdbms/commands.py	`89.79% <89.79%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf2a6a3...13cc3f1. Read the comment docs.

tjprescott

Please open the second PR with the test files.

derekbekoe · 2017-05-08T18:27:25Z

src/command_modules/azure-cli-rdbms/setup.py

+    logger.warn("Wheel is not available, disabling bdist_wheel hook")
+    cmdclass = {}
+
+VERSION = '0.0.1'


Add +dev to this version.
The version number will be changed just before releasing.

I was told that should not add +dev to the version, so i removed it.

tjprescott · 2017-05-08T19:23:18Z

@derekbekoe I will add a PR to add +dev to the setup file.

Initial commit for mysql and postgres command module

439090c

msftclas added the cla-not-required label May 8, 2017

troydai suggested changes May 8, 2017

View reviewed changes

Suna-MS added 2 commits May 9, 2017 01:53

Remove test files, will add them and recordings later

6543250

Remove test files in proj file

13cc3f1

derekbekoe modified the milestone: Build Milestone May 8, 2017

tjprescott approved these changes May 8, 2017

View reviewed changes

tjprescott changed the title ~~Initial commit for mysql and postgres command module~~ [RDBMS] MySQL/Postgres command support May 8, 2017

troydai approved these changes May 8, 2017

View reviewed changes

derekbekoe suggested changes May 8, 2017

View reviewed changes

tjprescott merged commit c88ef10 into Azure:master May 8, 2017

tjprescott mentioned this pull request May 8, 2017

[RDBMS] Add +dev to version #3244

Closed

haroldrandom added the cla-not-required label Oct 25, 2019


		import azure.cli.command_modules.rdbms.help # pylint: disable=unused-import

		__all__ = ['load_params', 'load_commands']



		add_helps("mysql", "MySQL")
		add_helps("postgres", "PostgreSQL")

Conversation

Suna-MS commented May 8, 2017 • edited by tjprescott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

Command Guidelines

Uh oh!

msftclas commented May 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troydai May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troydai May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott commented May 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Suna-MS commented May 8, 2017 •

edited by tjprescott

Loading

troydai May 8, 2017 •

edited

Loading

troydai May 8, 2017 •

edited

Loading

codecov-io commented May 8, 2017 •

edited

Loading