Skip to content

Conversation

@djyou
Copy link
Member

@djyou djyou commented Apr 26, 2018


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

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@djyou djyou requested a review from yugangw-msft April 26, 2018 23:09
@promptws
Copy link

View a preview at https://prompt.ws/r/Azure/azure-cli/6241
This is an experimental preview for @microsoft users.

@derekbekoe
Copy link
Member

The PR contains privates/azure_mgmt_containerregistry-2.0.0-py2.py3-none-any.whl.
This SDK should be published before this is merged.

@djyou djyou force-pushed the doyou/upstreamdev branch from 851ee94 to 011f3b8 Compare April 26, 2018 23:23
@djyou
Copy link
Member Author

djyou commented Apr 26, 2018

@derekbekoe Yes we wanted to start the review process while working on publishing the new SDK Azure/azure-sdk-for-python#1935. I also owe tests for the new commands and will add them shortly.

Copy link
Member

Choose a reason for hiding this comment

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

What's $PAT?
Would suggest a different example value here?

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 with $PAT

Copy link
Member

Choose a reason for hiding this comment

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

A FileCompleter may be useful here?

Copy link
Member

Choose a reason for hiding this comment

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

Much prefer using SDK values when possible.
Why this change? Does the SDK no longer document the skus available?

Copy link
Member

Choose a reason for hiding this comment

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

Use logger.warning for these user style messages.

Copy link
Member

Choose a reason for hiding this comment

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

Same with below on messages like 'Waiting for a build agent...' etc.

By using logger, the messages go to stderr instead of stdout.

@djyou
Copy link
Member Author

djyou commented Apr 27, 2018

@derekbekoe CI seems to be complaining the use of positional parameters. Could you please advise? We only have one such parameter.

@djyou
Copy link
Member Author

djyou commented Apr 27, 2018

@djyou
Copy link
Member Author

djyou commented Apr 27, 2018

/cc @mmacy @SteveLas for doc review and suggestions.

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.

Also, please show the -h output for the new commands.

Copy link
Member

Choose a reason for hiding this comment

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

You should not use this.

Copy link
Member

Choose a reason for hiding this comment

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

Why a debug message and potentially unexpected behavior rather than a CLIError?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a few cases that the response doesn't have the datetime yet. For example, if users trigger a build and immediately get the status of it before it finishes, we don't have a finish time here. This is table formatting path and we thought we can just show empty in the column rather than throwing an error.

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--if the user inputs an invalid format, then failing silently and having unexpected behavior is probably not better than throwing an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same reason as above handling response datetime. We don't format user inputs here. In this case, finish_time could be empty.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice this was for table format. Displaying blank is completely appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

@tjprescott
Copy link
Member

@djyou reviewing the stack trace, it looks like certain stages of the CI load parameters twice, which is why your one positional argument is being found twice and throwing an error. I'll make a PR to fix this.

@djyou djyou force-pushed the doyou/upstreamdev branch from bb9f9d5 to d26ab71 Compare April 30, 2018 04:45
@tjprescott
Copy link
Member

@derekbekoe I recommend we merge this. No idea what's going on with CI and it's recent constant timeouts on Integration.

@tjprescott tjprescott added this to the Sprint 36 (Build) milestone Apr 30, 2018
@tjprescott tjprescott merged commit ea1d946 into Azure:dev Apr 30, 2018
@djyou djyou deleted the doyou/upstreamdev branch April 30, 2018 23:40
@ghost ghost added Container Registry az acr and removed ACR labels Jun 25, 2020
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.

5 participants