Refactors amclient.py #48

ross-spencer · 2018-02-21T20:44:18Z

Creates new logging configuration script to reduce redundancy
Separates responsibility of amclient, defaults, and cli args
Cleans up argparse output using metavar
Adds Conformity to PEP8 to amclient.py and transfers.py
Returns user friendly string via getattr if function call fails in main
Improved error return from JSON request function in amclient.py
Adds .db and .lck files to .gitignore
Works at module and executable level
E402 flake8 warning ignored in Tox to enable module/exe to work
Captures connection errors in transfers.py better
Initial changes provides scope for future refactoring as required
Addresses Problem: The Automation Tools AMClient script could be easier to jump into and add functionality #47

Follows up #46 with more appropriate naming conventions for the branch.

ross-spencer · 2018-02-21T22:25:36Z

@jrwdunham @jraddaoui this should be a little bit better now.

@jrwdunham I hadn't anticipated the effect of renaming the branch, and so this is a new pull request. I have addressed the majority of the concerns in #46

Specifically:

Removed the global CONFIG variable in transfers.py
Handled long strings differently using parenthesis
Changed the import blocks to follow AM principles

What I haven't been able to address is primarily:

My removing the print_ inside main()'s getattr.

I had a look at using getattr to understand if there was a behaviour I was missing, but this seems to be used for exceptions i.e. when an attribute isn't there. Whereas the change I've implemented is to provide a way for the script to return a message to the user on the command over returning Null from the function if the call fails for some reason. This is for the arguments that the script can successfully find.

As far as I can tell, the output is equivalent. Maybe I'm missing something though? Idk, What do you think?

Additional Changes

Given @sevein's comments here, and time pressures, are you happy I create a new issue to add setuptools functionality at a later date? I think it would be a worthwhile addition.

jrwdunham · 2018-02-22T05:17:22Z

@ross-spencer The behaviour that is being missed is that AMClient().stdout is no longer being called. As a result, when one calls something like ./amclient.py aips test --ss-user-name=test --ss-url=http://127.0.0.1:62081/ --output-mode=Python, then the --output-mode=Python (self.output_mode) has no effect: the changes to main in this PR are printing the result to stdout without either pretty printing it (pprint) or converting it to JSON and printing it. This is what the stdout method does. See

automation-tools/transfers/amclient.py

Line 152 in 80d320b

self.stdout(getattr(self, method)())

In current master, you can compare this pretty-printed Python output:

$ ./amclient.py aips test --ss-user-name=test --ss-url=http://127.0.0.1:62081/ --output-mode=Python
[{u'current_full_path': u'/var/archivematica/sharedDirectory/www/AIPsStore/dac9/6ac9/f487/47fa/9082/ff74/4fc9/7792/hierarchy_with_empty_dir_1519246102-dac96ac9-f487-47fa-9082-ff744fc97792.7z',
  u'current_location': u'/api/v2/location/78cb7c7e-3cb4-4655-b09f-23ae9f647cae/',
  u'current_path': u'dac9/6ac9/f487/47fa/9082/ff74/4fc9/7792/hierarchy_with_empty_dir_1519246102-dac96ac9-f487-47fa-9082-ff744fc97792.7z',
 u'encrypted': False,
 u'misc_attributes': {},
 u'origin_pipeline': u'/api/v2/pipeline/d700f984-05ef-4351-9aac-0b85071ebf1a/',
 u'package_type': u'AIP',
 u'related_packages': [],
 u'replicas': [],
 u'replicated_package': None,
 u'resource_uri': u'/api/v2/file/dac96ac9-f487-47fa-9082-ff744fc97792/',
 u'size': 17038,
 u'status': u'UPLOADED',
 u'uuid': u'dac96ac9-f487-47fa-9082-ff744fc97792'}]

to this easily machine-readable JSON output:

$ ./amclient.py aips test --ss-user-name=test --ss-url=http://127.0.0.1:62081/ --output-mode=json
[{"status": "UPLOADED", "package_type": "AIP", "origin_pipeline": "/api/v2/pipeline/d700f984-05ef-4351-9aac-0b85071ebf1a/", "uuid": "dac96ac9-f487-47fa-9082-ff744fc97792", "replicas": [], "encrypted": false, "current_path": "dac9/6ac9/f487/47fa/9082/ff74/4fc9/7792/hierarchy_with_empty_dir_1519246102-dac96ac9-f487-47fa-9082-ff744fc97792.7z", "replicated_package": null, "misc_attributes": {}, "current_full_path": "/var/archivematica/sharedDirectory/www/AIPsStore/dac9/6ac9/f487/47fa/9082/ff74/4fc9/7792/hierarchy_with_empty_dir_1519246102-dac96ac9-f487-47fa-9082-ff744fc97792.7z", "current_location": "/api/v2/location/78cb7c7e-3cb4-4655-b09f-23ae9f647cae/", "related_packages": [], "size": 17038, "resource_uri": "/api/v2/file/dac96ac9-f487-47fa-9082-ff744fc97792/"}]

I would like to keep the old API including the output-mode flag.

Another issue is that your changes are checking whether the return value is a dict when different valid return values are possible. If I call a subcommand that returns a list (like the aips subcommand), then I go down the error path in the new code and that raises an uncaught exception because checking whether a list is in a dict raises a TypeError:

$ ./amclient.py aips test --ss-user-name=test --ss-url=http://127.0.0.1:62081/ --output-mode=json
Traceback (most recent call last):
  File "./amclient.py", line 413, in <module>
    main()
  File "./amclient.py", line 403, in main
    if res in errors.error_lookup:
TypeError: unhashable type: 'list'

Using the __getattr__ method allows us to define on the fly print_-prefixed methods for all of our subcommands without having to redundantly define them explicitly.

Why not keep your improved error-handling/printing logic but move it to the __getattr__ method so we can have the best of both worlds? Something like this would do it I think:

def __getattr__(self, name):
    if name.startswith('print_'):
        try:
            method = name.replace('print_', '', 1)
            res = getattr(self, method)()
            if isinstance(res, int):
                self.stdout(errors.error_lookup.get(
                    res,
                    errors.error_lookup[errors.ERR_AMCLIENT_UNKNOWN])
            else:
                self.stdout(getattr(self, method)())
        except:
            self.stdout(errors.error_lookup[errors.ERR_AMCLIENT_UNKNOWN])
    else:
        raise AttributeError('AMClient has no method {0}'.format(name))

And then restore the end of main to what it was:

try:
    getattr(am_client, 'print_{0}'.format(args.subcommand.replace('-', '_')))
except AttributeError:
    parser.print_help()
    sys.exit(0)

ross · 2018-02-22T05:19:32Z

I believe you want @ross-spencer rather than me 😀

jrwdunham · 2018-02-22T05:31:22Z

transfers/transfer.py

+    parser = argparse.ArgumentParser(description=__doc__,
+                                     formatter_class=rawformatter)
+    parser.add_argument('-u', '--user', metavar='USERNAME', required=True,
+                        help=('Username of the Archivematica dashboard user '


Just FYI, all these parens are unneeded because the kwarg=value expressions are all already in the method's parens. So this could just be

parser.add_argument('-u', '--user', metavar='USERNAME', required=True, help='Username of the Archivematica dashboard user ' 'to authenticate as.')

ross-spencer · 2018-02-22T15:46:46Z

Hi @jrwdunham I have added your snippet and it looks good (and makes sense). I'm afraid it's a case of RTFM for me here. i hadn't seen that stdout and __getattr__ were already part of the class, nor that --output-mode= was a sub-command argument. I thought the original call was a shortcut I hadn't seen before - the paradigm is a little different to how I've created executable files in Python before. This is good. I hope this wasn't too painful.

jrwdunham

Hi @ross-spencer. This is looking good. I have a few remaining comments.

jrwdunham · 2018-02-22T18:04:11Z

transfers/amclient.py

+            LOGGER.warning('%s Request to %s returned %s %s', method, url,
+                           response.status_code, response.reason)
+            LOGGER.debug('Response: %s', response.text)
+            return errors.ERR_INVALID_RESPONSE


Since we're no longer returning None from this func, I think you should change the :returns: line of the func docstring to reflect what can actually be returned.

Also, as you probably noticed, this _call_url_json is defined almost identically in both amclient.py and transfer.py. Do you think you could move your revised version into a new utils module that both of the public modules import?

Good spot on the docstring.

I like the utils.py idea as well. I have to change the error_lookup to be a little more robust - a function call rather than a naive dictionary lookup but I think that will work well across both scripts.

jrwdunham · 2018-02-22T18:14:58Z

transfers/amclient.py

+    # from any failed calls to the AM or SS servers.
+    def stdout(self, stuff):
+        """Print to stdout, either as JSON or pretty-printed Python."""
+        if self.output_mode == 'json':


This isn't your responsibility, but could you call lower() on self.stdout before the comparison here? This would allow the user to pass --output-mode=JSON and have it work as expected.

jrwdunham · 2018-02-22T18:18:38Z

transfers/amclientargs.py

+
+# Allow execution as an executable and the script to be run at package level
+# by ensuring that it can see itself.
+sys.path.append('../')


This smells a bit to me but I understand why you did it. I think maybe you should replace this with sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) in this file and in amclient.py and transfer.py. If you do, then we should be able to call these files in all of these ways:

$ pwd automation-tools $ ./transfers/amclient.py aips $ python transfers/amclient.py aips $ python -m transfers.amclient aips $ cd transfers $ ./amclient.py aips $ python amclient.py aips $ python -m amclient aips

As this PR is right now, you cannot call ./transfers/amclient.py aips or python transfers/amclient.py aips. What do you think?

Yup! Can confirm 👍

* Creates new logging configuration script to reduce redundancy * Separates responsibility of amclient, defaults, and cli args * Cleans up argparse output using metavar * Adds Conformity to PEP8 to amclient.py and transfers.py * Returns user friendly string via getattr if function call fails in main * Improved error return from JSON request function in amclient.py * Adds .db and .lck files to .gitignore * Works at module and executable level * E402 flake8 warning ignored in Tox to enable module/exe to work * Captures connection errors in transfers.py * Adds a utils module to handle json requests across scripts * Initial changes provides scope for future refactoring as required * Addresses #47

ross-spencer · 2018-03-02T19:03:05Z

@jrwdunham changes made! What do you think - looking better?

qubot force-pushed the dev/issue-47-refactor-am-client-responsibilities branch from 1cef7ce to 80d320b Compare February 21, 2018 22:14

ross-spencer requested review from jrwdunham and jraddaoui February 21, 2018 22:15

ross-spencer self-assigned this Feb 21, 2018

jrwdunham reviewed Feb 22, 2018

View reviewed changes

qubot force-pushed the dev/issue-47-refactor-am-client-responsibilities branch from 80d320b to 451ca2f Compare February 22, 2018 15:38

jrwdunham suggested changes Feb 22, 2018

View reviewed changes

ross-spencer mentioned this pull request Mar 2, 2018

Problem: Consider exposing status codes and response.text to calling functions #50

Open

qubot force-pushed the dev/issue-47-refactor-am-client-responsibilities branch 2 times, most recently from e4051bf to 43c988c Compare March 2, 2018 18:41

qubot force-pushed the dev/issue-47-refactor-am-client-responsibilities branch from 43c988c to 699b018 Compare March 2, 2018 18:59

jrwdunham approved these changes Mar 5, 2018

View reviewed changes

qubot merged commit 699b018 into master Mar 5, 2018

qubot deleted the dev/issue-47-refactor-am-client-responsibilities branch March 5, 2018 23:27

hakamine mentioned this pull request Mar 13, 2018

Problem: amclient does not use log file location specified by --log-file #51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors amclient.py #48

Refactors amclient.py #48

ross-spencer commented Feb 21, 2018

ross-spencer commented Feb 21, 2018 •

edited

Loading

jrwdunham commented Feb 22, 2018 •

edited

Loading

ross commented Feb 22, 2018

jrwdunham Feb 22, 2018

ross-spencer commented Feb 22, 2018 •

edited

Loading

jrwdunham left a comment

jrwdunham Feb 22, 2018

jrwdunham Feb 22, 2018

ross-spencer Mar 1, 2018

jrwdunham Feb 22, 2018

ross-spencer Mar 1, 2018

jrwdunham Feb 22, 2018

ross-spencer Mar 2, 2018

ross-spencer commented Mar 2, 2018

Refactors amclient.py #48

Refactors amclient.py #48

Conversation

ross-spencer commented Feb 21, 2018

ross-spencer commented Feb 21, 2018 • edited Loading

jrwdunham commented Feb 22, 2018 • edited Loading

ross commented Feb 22, 2018

Choose a reason for hiding this comment

ross-spencer commented Feb 22, 2018 • edited Loading

jrwdunham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ross-spencer commented Mar 2, 2018

ross-spencer commented Feb 21, 2018 •

edited

Loading

jrwdunham commented Feb 22, 2018 •

edited

Loading

ross-spencer commented Feb 22, 2018 •

edited

Loading