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

[Advisor] Issues to resolve prior to Advisor module GA #5343

Closed
5 of 13 tasks
tjprescott opened this issue Jan 18, 2018 · 4 comments
Closed
5 of 13 tasks

[Advisor] Issues to resolve prior to Advisor module GA #5343

tjprescott opened this issue Jan 18, 2018 · 4 comments
Assignees
Labels
Advisor Fit&Finish Service Attention This issue is responsible by Azure service team.

Comments

@tjprescott
Copy link
Member

tjprescott commented Jan 18, 2018

High Priority

  • advisor configuration get should be split into advisor configuration list since it returns a collection. --resource-group should not be an optional parameter since, as far as I can tell, a resource group can only have a single configuration.
  • advisor configuration get returns a format with nextLink and value. It should only return the contents of value. Any paging should be automatically handled by autorest.
  • The commands lack examples. When I try to create a configuration for a resource group I just get an object with code: ImproperScope. The help text should also make clear that low-cpu-threshold can only use used for the subscription, not per RG.
  • advisor configuration set should be called advisor configuration update as it displays PATCH-like behavior, not PUT-like behavior.
  • For me, az advisor recommendation generate produces no output. If there are no recommendations, something to that effect should be output.
  • az advisor configuration set should return the updated object instead of forcing the user to make a subsequent call to get.
  • The relationship between recommendation generate and recommendation list is not clear to me. Generate appears to do nothing and list returns an empty collection. Speculating here based on how the configuration set command works, I suspect that generate should immediately follow up with a list when it completes.
  • The help text for --ids in the recommendation enable/disable commands makes reference to "Resource Id" arguments. However, these aren't in a Resource Id Arguments group. You should be able to specify the resource by names (resource group and configuration name). Additionally, the --ids parameter doesn't seem to work, if I give GUIDs or full ARM resource IDs...

Recommended

  • recommendation: tests should use the built-in checks syntax instead of getting outputs in json and making assertions on them.
  • If there can only be a single configuration object per resource group, you could also simply make the enable/disable commands accept the resource group name (if you can enable/disable the subscription level one, there would need to be a mechanism for that).
  • The recommendation enable/disable commands could logically be folded into a single command.
  • There should be an advisor configuration show command that optionally accepts resource group and shows the single configuration for that group (or if omitted, the subscription config).
  • Since exclude is a property of a an Advisor configuration object, --exclude should be a three-state flag and --include is not needed.

Environment summary

Install Method (e.g. pip, interactive script, apt-get, Docker, MSI, edge build) / CLI version (az --version) / OS version / Shell Type (e.g. bash, cmd.exe, Bash on Windows)

azure-cli 2.0.25
@Prasanna-Padmanabhan
Copy link
Contributor

Addressed most of the feedback by making the parameter changes/renames.

@bsiegel bsiegel added the Service Attention This issue is responsible by Azure service team. label Sep 26, 2018
@tjprescott tjprescott reopened this Apr 8, 2019
@tjprescott
Copy link
Member Author

The issue isn't resolved until all points are addressed. Reopening for tracking purposes.

@Prasanna-Padmanabhan
Copy link
Contributor

All of them were addressed back in Jan 2018. You can look through your email for more details if you like :)

@tjprescott
Copy link
Member Author

This module is now GA.

@haroldrandom haroldrandom added Advisor Fit&Finish Service Attention This issue is responsible by Azure service team. labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Advisor Fit&Finish Service Attention This issue is responsible by Azure service team.
Projects
None yet
Development

No branches or pull requests

4 participants