support listing, retrieving, and updating Configure Tower in Tower setting values by ryanpetrello · Pull Request #252 · ansible/tower-cli

ryanpetrello · 2017-03-01T22:47:33Z

No description provided.

jangsutsr · 2017-03-02T14:57:14Z

tower_cli/models/base.py

        # rather than URLs, since this endpoint just takes integers.
        for key in ('next', 'previous'):
-            if not response[key]:
+            if not response.get(key):


This alone will not prevent AttributeError, response.get(key, None) will.

response is a dict, so it would be a KeyError. dict.get falls back to None, so I think this should work: https://docs.python.org/2/library/stdtypes.html#dict.get

python -c "print {}.get('missing') is None"

Indeed. I didn't try get without default value before, sorry.

AlanCoding · 2017-03-02T15:11:25Z

tower_cli/resources/setting.py

+    @resources.command(ignore_defaults=True)
+    def modify(self, pk, value, **kwargs):
+        """Modify an already existing object."""
+        prev_value = self.get(pk)['value']


If this were to behave like other tower-cli modify commands, I think that it would write a debug statement if prev_value==value, and avoid doing the patch.

I thought about this and one area that this falls apart is encrypted field values - we can't really tell if those changed :/

That's true. My personal vote is a no-op in these cases unless it is encrypted (I want to favor fewer database changes with logging settings in mind).

AlanCoding · 2017-03-02T15:13:36Z

tower_cli/resources/setting.py

+            raise exc.NotFound('The requested object could not be found.')
+
+    @resources.command(ignore_defaults=True)
+    def modify(self, pk, value, **kwargs):


I take it that the pk is the string representing the key here. Given this code, I don't think the category will be relevant, so my gut tells me that use_fields_as_options should be set to False and that **kwargs could be safely removed. We also want to advertise that there's just one specific use-case we want to support for the command.

Don't we want use_fields_as_options so that the value can be specified via --value ?

Here you have value as an argument, and if it were an option (of the command like --value) I would expected it to be a keyword argument to the function. On the other hand, if you wanted to keep it as an argument you might consider decorating with @click.argument.

Also, use_fields_as_options can take a list, putting value into that would be one possibility.

AlanCoding · 2017-03-02T15:17:14Z

tower_cli/models/base.py

-            newattrs['endpoint'] = '/' + newattrs['endpoint']
-        if not newattrs['endpoint'].endswith('/'):
-            newattrs['endpoint'] += '/'
+        if isinstance(newattrs['endpoint'], basestring):


It looks like some python environment hit ./tower_cli/models/base.py:118:45: F821 undefined name 'basestring'. I think that jangsutsr was working on a solution to that problem, and he's probably figured it out by now.

That's true. It will fail python3, where basestring is removed. This is also the reason for travis test failures. Try using six.text_type or whatever is suitable.

* if a setting is modified, but the provided value isn't different than the current setting, don't actually PATCH * if an encrypted setting is modified, *always* PATCH (and show the value as changed, because there's really no way to tell for sure)

coveralls · 2017-03-02T20:28:50Z

Changes Unknown when pulling 75bbc93 on ryanpetrello:settings-cli into ** on ansible:master**.

coveralls · 2017-03-02T20:31:17Z

Changes Unknown when pulling d33cec8 on ryanpetrello:settings-cli into ** on ansible:master**.

coveralls · 2017-03-02T20:33:28Z

Changes Unknown when pulling 966c4e8 on ryanpetrello:settings-cli into ** on ansible:master**.

this changes from the invocation from: $ tower-cli setting modify SETTING_NAME --value=1812 to: $ tower-cli setting modify SETTING_NAME 1812

coveralls · 2017-03-02T20:41:16Z

Changes Unknown when pulling 73d1473 on ryanpetrello:settings-cli into ** on ansible:master**.

AlanCoding · 2017-03-02T20:46:14Z

tower_cli/resources/setting.py

+        finally:
+            self.custom_category = None
+        return {
+            'results': [{'id': k, 'value': v} for k, v in result.items()]


this also affects just doing tower-cli setting list

AlanCoding · 2017-03-02T21:07:50Z

tower_cli/resources/setting.py

+        try:
+            return next(s for s in self.list()['results'] if s['id'] == pk)
+        except StopIteration:
+            raise exc.NotFound('The requested object could not be found.')


I checked out your branch and this works:

tower-cli setting get SOCIAL_AUTH_SAML_ORGANIZATION_MAP

This does not work:

tower-cli setting get --value=SOCIAL_AUTH_SAML_ORGANIZATION_MAP

That's fine, I somewhat prefer the one that works, but the help text:

$ tower-cli setting get Usage: tower-cli setting get [OPTIONS] [ID] Return one and exactly one setting Options: --value TEXT [REQUIRED] The value field. -h, --tower-host TEXT The location of the Ansible Tower host. HTTPS is assumed as the protocol unless "http://" is explicitly provided. This will take precedence over a host provided to `tower config`, if any. -u, --tower-username TEXT Username to use to authenticate to Ansible Tower. This will take precedence over a username provided to `tower config`, if any. -p, --tower-password TEXT Password to use to authenticate to Ansible Tower. This will take precedence over a password provided to `tower config`, if any. -f, --format [human|json|yaml] Output format. The "human" format is intended for humans reading output on the CLI; the "json" and "yaml" formats provide more data. -v, --verbose Show information about requests being made. --description-on Show description in human-formatted output. --insecure Turn off insecure connection warnings. Set config verify_ssl to make this permanent. --certificate TEXT Path to a custom certificate file that will be used throughout the command. Overwritten by --insecure flag if set. --help Show this message and exit.

I have a sense of what your edits to base.py did, and since the other standard use still works I'm fine with that, but you'll see that the help text points us to the --value usage. Because of that, use_fields_as_options may need to be revisited.

coveralls · 2017-03-02T21:24:31Z

Changes Unknown when pulling bed853f on ryanpetrello:settings-cli into ** on ansible:master**.

AlanCoding

Tested, and the comments here are addressed sufficiently. Also consider issue 246, which could be added to this or implemented in a separate PR.

AlanCoding · 2017-03-03T22:07:38Z

This is giving me trouble now:

$ tower-cli setting modify AUTH_LDAP_CONNECTION_OPTIONS '{"OPT_NETWORK_TIMEOUT": 40, "OPT_REFERRALS": 0}'
Error: The Tower server claims it was sent a bad request.

PATCH https://localhost:8043/api/v1/settings/all/
Params: None
Data: {"AUTH_LDAP_CONNECTION_OPTIONS": "{\"OPT_NETWORK_TIMEOUT\": 40, \"OPT_REFERRALS\": 0}"}

Response: {"AUTH_LDAP_CONNECTION_OPTIONS":["Expected a dictionary of items but got type \"unicode\"."]}

$ tower-cli setting modify AUTH_LDAP_CONNECTION_OPTIONS {"OPT_NETWORK_TIMEOUT": 40, "OPT_REFERRALS": 0}
Usage: tower-cli setting modify [OPTIONS] SETTING [VALUE]

Error: Got unexpected extra arguments (40, OPT_REFERRALS: 0})

How can we edit JSON settings?

AlanCoding

I want to get JSON content working first, I might submit a patch

coveralls · 2017-03-06T14:55:35Z

Changes Unknown when pulling 121d2a0 on ryanpetrello:settings-cli into ** on ansible:master**.

see: ansible#246

AlanCoding

LGTM

coveralls · 2017-03-06T21:48:53Z

Coverage increased (+0.1%) to 94.514% when pulling ecc9df2 on ryanpetrello:settings-cli into 4b88052 on ansible:master.

jlaska added the review label Mar 1, 2017

ryanpetrello force-pushed the settings-cli branch 2 times, most recently from 031dfcf to c2d7a46 Compare March 1, 2017 22:57

ryanpetrello requested review from AlanCoding and wwitzel3 March 1, 2017 23:03

ryanpetrello added 6 commits March 2, 2017 08:50

support listing, retrieving, and updating CTinT setting values

4b449b6

add the ability to list/filter settings by category

77a8bff

add tests for setting category filtering

d27c944

remove some confusing flags from settings resource commands

9668648

fixup flake8 warnings

e7e925a

only show changes for setting modification if the value actually changed

9d47e83

ryanpetrello force-pushed the settings-cli branch from c2d7a46 to 9d47e83 Compare March 2, 2017 13:56

jangsutsr reviewed Mar 2, 2017

View reviewed changes

AlanCoding reviewed Mar 2, 2017

View reviewed changes

AlanCoding added this to the release_3.1.0 milestone Mar 2, 2017

ryanpetrello added 2 commits March 2, 2017 14:48

fixup py3 compat

6d76074

improve setting change detection

75bbc93

* if a setting is modified, but the provided value isn't different than the current setting, don't actually PATCH * if an encrypted setting is modified, *always* PATCH (and show the value as changed, because there's really no way to tell for sure)

ryanpetrello added 2 commits March 2, 2017 15:28

add a test for unicode setting modification

d33cec8

py26 compat fixup

966c4e8

simplify setting modification to use a fixed arg rather than a flag

73d1473

this changes from the invocation from: $ tower-cli setting modify SETTING_NAME --value=1812 to: $ tower-cli setting modify SETTING_NAME 1812

AlanCoding reviewed Mar 2, 2017

View reviewed changes

correct help text for tower-cli setting get

bed853f

AlanCoding approved these changes Mar 3, 2017

View reviewed changes

AlanCoding suggested changes Mar 3, 2017

View reviewed changes

properly coerce setting values based on their type defined in OPTIONS

121d2a0

allow license uploads

ecc9df2

see: ansible#246

ryanpetrello force-pushed the settings-cli branch from e54578f to ecc9df2 Compare March 6, 2017 21:46

AlanCoding approved these changes Mar 6, 2017

View reviewed changes

ryanpetrello merged commit c5d246a into ansible:master Mar 6, 2017

jlaska removed the review label Mar 6, 2017

ryanpetrello mentioned this pull request Mar 6, 2017

Allow JSON settings and license uploads ryanpetrello/tower-cli#1

Closed

Conversation

ryanpetrello commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanCoding left a comment

Choose a reason for hiding this comment

Uh oh!

AlanCoding commented Mar 3, 2017

Uh oh!

AlanCoding left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanCoding left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Mar 2, 2017 •

edited

Loading

coveralls commented Mar 2, 2017 •

edited

Loading

coveralls commented Mar 2, 2017 •

edited

Loading

coveralls commented Mar 2, 2017 •

edited

Loading

coveralls commented Mar 2, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading