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

Generic Update enhancements. #740

Merged
merged 2 commits into from
Aug 30, 2016
Merged

Generic Update enhancements. #740

merged 2 commits into from
Aug 30, 2016

Conversation

tjprescott
Copy link
Member

@tjprescott tjprescott commented Aug 24, 2016

This PR fixes #686 and fixes #728.

Allows indexing into a collection via unique key instead of just index.

options = options.keys()
options = sorted([make_camel_case(x) for x in options])
elif isinstance(options, list):
options = 'index into the collection with {}[<index>] or [<key=value>]'.format(parent)
Copy link
Contributor

@BurtBiel BurtBiel Aug 24, 2016

Choose a reason for hiding this comment

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

{}[] or [ [](start = 49, length = 17)

this might be clearer if we said index into collection {} with [... #Resolved

@tjprescott
Copy link
Member Author

tjprescott commented Aug 27, 2016

@BurtBiel I went ahead and gave the error when setting on a non-existent property. I verified that setting tags still works whether it is null, {} or existing. #Resolved

@@ -14,6 +14,7 @@

#pylint:disable=invalid-sequence-index
#pylint:disable=unsubscriptable-object
#pylint:disable=line-too-long
Copy link
Contributor

@BurtBiel BurtBiel Aug 29, 2016

Choose a reason for hiding this comment

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

there are only 2 lines over limit, we might as well break them and avoid this disable #Resolved

@BurtBiel
Copy link
Contributor

Is this scenario supported?

start with:
{
foo: {}
}

--update bar=new
=>
{
foo: {}
bar: "new"
}

If yes, we should have all the cases covered (that would be good to add a test for, I think I missed that one). If no, we should investigate more. There are definitely cases where properties are missing until they are set, I saw one the other day and should have written it down. We support adding properties to sub-dictionaries correct? If so, we should be able to support adding to the root as well.


In reply to: 242885056 [](ancestors = 242885056)

@tjprescott
Copy link
Member Author

That scenario is definitely not supported with the current PR, because in every case I've seen, what you would actually get is apparent success but the object "{ foo: {} }" i.e. it will completely discard you setting a nonexistent property. I've always seen unset properties as null or empty. If there is a case where this is necessary then I will remove the check to make sure the property exists and revert those lines to the original behavior.

You can add properties to a subdictionary if it is a dictionary, but the root object is rarely (ever?) a dictionary.


In reply to: 243224907 [](ancestors = 243224907,242885056)

@tjprescott
Copy link
Member Author

I added a warning log if the property does not exist but still set the property. This will enable any scenario that requires setting a property that doesn't appear in the object, but at least give some information if someone attempts to set a property and is frustrated that the update didn't "take". This is one of the many reasons I don't recommend generic update being our "normal" update method. :)


In reply to: 243255780 [](ancestors = 243255780,243224907,242885056)

@BurtBiel
Copy link
Contributor

:shipit:

@tjprescott tjprescott merged commit ab63040 into Azure:master Aug 30, 2016
@tjprescott tjprescott deleted the GenericUpdateFixes branch August 30, 2016 17:14
johanste added a commit that referenced this pull request Aug 31, 2016
* Provide support for reading a single parameter value from file

Values prefixed with @ will be replaced with the content of the file immediately following the @-sign.

The pseudo-name '-' is used to indicate stdin.

* Fix accidentally deleted space (which made pylint angry)

* enable diagnostics and extensions in vmss profile (#737)

* Expose generic update parameters on existing custom updates (#735)

* Convert all updates to cli_generic_update_commands.

* Code review fixes.

* Remove inline CliArgumentType(...) registrations (#744)

* Eliminates anti-pattern of creating CliArgumentTypes inline during parameter registration.

* Minor fixes.

* Add NAT Rules to VMSS create that allow SSH/RDP by default (#743)

* Update regular expression to fix issue #147 (#747)

* Support colored output on Windows (#756)

* [Help] Help Fixes and Enhancements (#736)

* Fix issue #680.

* Initial version of help dump. Logic to extract summary.

* Fix issues #365 and #447.

* Code review fixes.

* Code review fixes.

* Fix folded parameter logic to make new optional. Update existing uses to (#766)

work with new logic.

* Treat CloudError like a CLIError (don't print stack trace) (#759)

* 'az configure' experience (default output format) (#739)

* 'az configure' experience

- Allows user to set default output type.
- Infrastructure for CLI configurations also implemented.

* Add license header & fix deprecated method

* Wrap SafeConfigParser instead of extending from it

* Code review feedback changes

* Table format revisions (#748)

* Table format revisions

- Table output is generic and automatically extracts fields from the result.
- No longer use simple_output_query as it was specific for commands and as packages will support multiple API versions, this solution is no longer feasible.

* Support callable again for table format after discussion

When a callable is set for a command, it will be used as long as there is no query active.
If there's a query active, the callable will not be used so the user has to specify a full query that can generate an appropriate table.

* Remove 'starting' message for long running operations (#767)

* Remove 'starting' message for long running operations

* Change name of poll_interval_ms as ambiguous and also change logger messages that referred to this interval

* Catch EOF to minimize stack trace craziness. (#768)

* Change simple_output_query to be table_transformer (#771)

* [Resource] Resource Tag Fix (issue 763) (#765)

* Use 'ignore_type' within resource module. Fix issue 763.

* Code review fixes.

* Only delete certain files in yml output folder during Document CI (#762)

* Update README Docker instructions. (#776)

* Fixes #772 (#779)

* Handle BrokenPipeError (#775)

* Fix issue 770.

* Fix undeclared-variable

* update a few command descriptions (#781)

* VM issue: fix #749, #753, #575 (#778)

* Add commands for Azure IoT (#730)

* Add commands for Azure IoT

* 1.Include files into azure-cli.pyproj; 2. Update license info in setup.py

* add missing pylint directive

* resolve review comments

* resolve comments

* fix typo

* [Document CI] Handle Robocopy exit code (#786)

* Generic Update enhancements. (#740)

* Fix issues #728 and #686

* Code review fixes.

* Fix Error if Non-existent Storage Account Specified (#787)

* Fix issue #784.

* Code review fix.

* vm: enable autoUpgradeMinorVersion by default (#791)

* cmd: use plurals for list-* command naming (#792)

Committer: yugangw-msft

* Ensure docstring help with unrecognized unicode characters still displays correctly. (#795)

* Provide support for reading a single parameter value from file

Values prefixed with @ will be replaced with the content of the file immediately following the @-sign.

The pseudo-name '-' is used to indicate stdin.

* Fix accidentally deleted space (which made pylint angry)
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.

4 participants