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

[ACS] format default dns_name_prefix #4237

Merged
merged 5 commits into from
Aug 17, 2017

Conversation

rjtsdl
Copy link
Contributor

@rjtsdl rjtsdl commented Aug 16, 2017


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.

(see Authoring Command Modules)

@msftclas
Copy link

@rjtsdl,
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

@rjtsdl
Copy link
Contributor Author

rjtsdl commented Aug 16, 2017

fix #4116

@rjtsdl
Copy link
Contributor Author

rjtsdl commented Aug 16, 2017

@anhowe TAL

@codecov-io
Copy link

codecov-io commented Aug 16, 2017

Codecov Report

Merging #4237 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4237      +/-   ##
==========================================
+ Coverage   70.05%   70.05%   +<.01%     
==========================================
  Files         475      475              
  Lines       29605    29612       +7     
  Branches     4539     4540       +1     
==========================================
+ Hits        20739    20746       +7     
  Misses       7419     7419              
  Partials     1447     1447
Impacted Files Coverage Δ
...re-cli-acs/azure/cli/command_modules/acs/custom.py 44.26% <100%> (+0.7%) ⬆️
...e-cli-acs/azure/cli/command_modules/acs/_params.py 76.71% <100%> (ø) ⬆️

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 bdeaa6e...c57559e. Read the comment docs.

@@ -81,7 +81,7 @@ def _get_feature_in_preview_message():
# some admin names are prohibited in acs, such as root, admin, etc. Because we have no control on the orchestrators, so default to a safe name.
register_cli_argument('acs', 'admin_username', options_list=('--admin-username',), default='azureuser', required=False)
register_cli_argument('acs', 'api_version', options_list=('--api-version',), required=False, help=_get_feature_in_preview_message() + 'Use API version of ACS to perform az acs operations. Available options: 2017-01-31, 2017-07-01. Default to use the latest version for the location')
register_cli_argument('acs', 'dns_name_prefix', options_list=('--dns-prefix', '-d'))
register_cli_argument('acs', 'dns_name_prefix', options_list=('--dns-prefix', '-d'), help='default use the format of <clustername>-<resourcegroupname>-<subid>, will trim the length and replace sensitive charactors if needed')
Copy link
Member

Choose a reason for hiding this comment

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

typo: characters

if not name_part[0].isalpha():
name_part = (str('a') + name_part)[0:10]
resource_group_part = re.sub('[^A-Za-z0-9-]', '', resource_group_name)[0:16]
dns_name_prefix = '{}-{}-{}'.format(name_part, resource_group_part, subscription_id[0:6])
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend extracting this logic to method and adding a few unit tests.

@rjtsdl
Copy link
Contributor Author

rjtsdl commented Aug 16, 2017

@tjprescott , please help merge if you approve :)

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@tjprescott tjprescott merged commit ec20314 into Azure:master Aug 17, 2017
@rjtsdl rjtsdl deleted the jiren-formatdnsprefix branch August 17, 2017 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants