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

{ACR}: expose --expiration from token commands #13451

Merged
merged 2 commits into from
May 15, 2020

Conversation

yugangw-msft
Copy link
Contributor

Description
See history entries below

History Notes
az acr token create: expose --expiration for accurate time
az acr token credential generate: expose --expiration for accurate time
az acr show-endpoints: warn if the dedicated endpoint is not enabled

@yugangw-msft yugangw-msft changed the title [ACR]: expose --expiration from token commands {ACR}: expose --expiration from token commands May 12, 2020
@@ -167,7 +167,7 @@ def load_command_table(self, _): # pylint: disable=too-many-statements
g.command('delete', 'acr_delete')
g.show_command('show', 'acr_show')
g.command('show-usage', 'acr_show_usage', table_transformer=usage_output_format)
g.command('show-endpoints', 'acr_show_endpoints', table_transformer=endpoints_output_format, is_preview=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do apologize for the back and forth, but we decide to make it GA

@yugangw-msft yugangw-msft added this to the S169 - For Build milestone May 12, 2020
@yonzhan yonzhan requested a review from qwordy May 12, 2020 10:54
@yonzhan
Copy link
Collaborator

yonzhan commented May 12, 2020

ACR

@yungezz yungezz removed the request for review from qwordy May 12, 2020 13:27
@yungezz yungezz assigned fengzhou-msft and unassigned yungezz May 12, 2020
@@ -332,6 +333,11 @@ def load_arguments(self, _): # pylint: disable=too-many-statements
c.argument('expiration_in_days', options_list=['--expiration-in-days', c.deprecate(target='--days', redirect='--expiration-in-days', hide=True)],
help='Number of days for which the credentials will be valid. If not specified, the expiration will default to the max value "9999-12-31T23:59:59.999999+00:00"', type=int, required=False)

for scope in ['acr token create', 'acr token credential generate']:
with self.argument_context(scope) as c:
c.argument('expiration', validator=validate_expiration_time,
Copy link
Member

Choose a reason for hiding this comment

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

seems there're different expiration unit/measure: expire in days, expire at exact date etc.

Copy link
Member

Choose a reason for hiding this comment

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

consider expire_at as option name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yungezz, Yes expiration is for exact date, the expiration-in-days is for days to make the input easier.

@fengzhou-msft, I name it as expiration for wording consistency with --expiration-in-days; or I can do --expire-in-days and --expire-at, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

I think --expire-in-days and --expire-at is more clear.

Copy link
Member

Choose a reason for hiding this comment

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

@yugangw-msft let me know whether you would make the change. Either is fine as users can always know the meaning of the options from help messages.

@yugangw-msft
Copy link
Contributor Author

@fengzhou-msft , the CI failures was not caused by this PR. Could you please cross check and merge when ready?

@fengzhou-msft
Copy link
Member

@fengzhou-msft , the CI failures was not caused by this PR. Could you please cross check and merge when ready?

Can you merge the latest dev branch? It has the fix for the CI error.

@yugangw-msft
Copy link
Contributor Author

Can you merge the latest dev branch? It has the fix for the CI error.

@fengzhou-msft, done

@fengzhou-msft fengzhou-msft merged commit 50cedea into Azure:dev May 15, 2020
@fengzhou-msft
Copy link
Member

BTW, you don't need the colon if you don't have BREAKING CHANGE or put a command name in the title. See the title format here.

derekbekoe added a commit to derekbekoe/azure-cli that referenced this pull request May 18, 2020
* 'dev' of https://github.com/Azure/azure-cli: (85 commits)
  {Monitor} az monitor autoscale profile show: bug fix (Azure#13515)
  [Monitor] az monitor diagnostic-settings create: support --export-to-specific-resource argument (Azure#13544)
  [aro]Add tests for generate_random_id function (Azure#13522)
  [ARO]Change CLIError to correct flag for --worker-vm-disk-size-gb (Azure#13439)
  [Packaging] Add Ubuntu Focal Package (Azure#13491)
  Update parse_zone_file.py (Azure#13377)
  {Release} Upgrade to Azure CLI 2.6.0 (Azure#13542)
  fix (Azure#13511)
  {Resource} Add example to tell how to pass array to --parameters (Azure#13510)
  [NetAppFiles] Anf 5207 Bugfix - add missing snapshot restore function (Azure#13481)
  {Network} az network nic create: Refine help message (Azure#13513)
  [Storage] az storage blob sync: Fix the incorrect error message when azcopy cannot find the installation location (Azure#9576)  (Azure#13104)
  Support --connect-string for az storage blob sync (Azure#13135)
  Modify the deprecate information for deployment operation (Azure#13390)
  [AppService] Onboard local context for app service (Azure#12984)
  {Monitor} az monitor metrics alert: refine severity explanation (Azure#13512)
  [IoT Hub] Support for 2020-03-01 API and Network Isolation commands (Azure#13467)
  [Compute] az vm list-skus: Update --zone behavior, return all type skus now (Azure#13470)
  Fix that cli doe not fail when user only specifies Windows password (Azure#13418)
  {ACR}: expose --expiration from token commands (Azure#13451)
  ...
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.

4 participants