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.

Sign up for GitHub

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

Jump to bottom

Create a 1.0.0-rc package #1

Merged
merged 1 commit into from
Mar 8, 2019
Merged

Create a 1.0.0-rc package #1

merged 1 commit into from
Mar 8, 2019

Conversation

ross-spencer
Copy link
Contributor

@ross-spencer ross-spencer commented Mar 2, 2019 •
edited
Loading

A package can be downloaded using pip install amclient it can then be invoked from the command line: $ amclient or imported as a Python module, recommended is to import: from amclient import AMClient which will bring into your code the AMClient class to mimic its usage in the automation-tools or amauat.

There are some final steps to push this over the line, including code review of the work here. I should draw the reader's attention to minor changes to the amclient modules to function as a package. Additional contributions in this PR are all towards the goal of packaging and making amclient easier to import for users.

Checklist here: archivematica/Issues#543

The package release-candidate is here: https://pypi.org/project/amclient/

Notes

  • I have tried to make commits follow a logical order to make them easier for code-review but I am happy to chat in person or discuss below anything that might not be clear.
  • I haven't brought all of the README into the package, as I would like the opportunity to discuss its value where a lot of this can be provided to the user by simply running the command without arguments. See the previous, detailed docs here: https://github.com/artefactual/automation-tools#archivematica-client.
  • I have tested the integration of the package with the automation tools and amauat and currently it does seem to function as a direct replacement for the submodule/relative imports used in both those utilities.
  • We can refine this package further, and for some of the more idiomatic changes we might make, such as, Problem: Response handling in AMClient.py utilities module isn't idiomatic Python archivematica/Issues#488 then a version 2.0.0 of this package would be a good way to make changes in parallel with these libraries.
  • Example use in code:
Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from amclient import AMClient
>>> am = AMClient()
>>> am.ss_url = "http://127.0.0.1:62081"
>>> am.ss_user_name = "test"
>>> am.ss_api_key = "test"
>>> am.list_storage_locations()
...json is output here...

Connects to archivematica/Issues#467
Connects to archivematica/Issues#477
Connects to artefactual/automation-tools#73

@ross-spencer ross-spencer self-assigned this Mar 2, 2019
@ross-spencer ross-spencer force-pushed the dev/issue-477-package-amclient branch from 2c9c503 to 5bb6aec Compare March 2, 2019 15:34
@ross-spencer ross-spencer force-pushed the dev/issue-477-package-amclient branch 3 times, most recently from a0ad436 to fd1e7e3 Compare March 3, 2019 16:29
@ross-spencer ross-spencer changed the title WIP: 1.0.0-rc Create a 1.0.0-rc package Mar 3, 2019
helenst
helenst reviewed Mar 5, 2019
View reviewed changes
amclient/defaults.py Outdated Show resolved Hide resolved
amclient/defaults.py Outdated
@@ -20,7 +22,7 @@
THIS_DIR = os.path.abspath(os.path.dirname(__file__))

# Global for logfile if not set.
AMCLIENT_LOG_FILE = os.path.join('/tmp/amclient.log')
AMCLIENT_LOG_FILE = os.path.join("{}".format(mkdtemp()), 'amclient.log')
Copy link

Choose a reason for hiding this comment

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

Why is format used here? (rather than using the output of mkdtemp() directly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, I think this is just leftover from my testing the mkdtemp mechanism.

requirements/base.txt Outdated
@@ -0,0 +1,3 @@
requests<3.0
six
urllib3
Copy link

Choose a reason for hiding this comment

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

Can these be version-pinned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, have you suggested versions to pin them to?

Copy link

Choose a reason for hiding this comment

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

I'd be inclined to go for latest versions (six==1.12.0 and urllib3==1.24.1). These are what it installed for me and they appear to work in both python 2 and 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! I've added those, so if you're happy with the CR changes, I'll rebase, and then seek to merge these into the master branch and create a release.

tox.ini Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
helenst
helenst reviewed Mar 5, 2019
View reviewed changes
Copy link

@helenst helenst left a comment

Choose a reason for hiding this comment

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

I've been testing this out, it seems to work well.

@ross-spencer
Copy link
Contributor Author

Hi @helenst I've made most of those changes. I just have the version pinning to do, but I haven't really an instinct about it, so perhaps it is something we can react to if there is a problem in future? But let me know.

@ross-spencer ross-spencer requested a review from helenst March 6, 2019 21:09
helenst
helenst approved these changes Mar 8, 2019
View reviewed changes
Copy link

@helenst helenst left a comment

Choose a reason for hiding this comment

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

This looks great to me 👍

@ross-spencer
Create a 1.0.0-rc of amclient
c032974 
This commit brings all of the original assets of the automation
tools' amclient (Archivematica API client) into a single pypi
package. The first release is a drop-in replacement for that work
in the modules and tools that already make use of it.
@ross-spencer ross-spencer force-pushed the dev/issue-477-package-amclient branch from e955bf1 to c032974 Compare March 8, 2019 19:39
@ross-spencer ross-spencer merged commit c032974 into master Mar 8, 2019
@ross-spencer ross-spencer deleted the dev/issue-477-package-amclient branch March 8, 2019 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helenst helenst helenst approved these changes

@replaceafill replaceafill Awaiting requested review from replaceafill

Assignees

@ross-spencer ross-spencer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ross-spencer @helenst