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

Replace bash scripts with python utility package #26

Merged

Conversation

aloysbaillet
Copy link
Contributor

My apologies in advance for another big PR...
The growing number of bash scripts in this repo was making me uncomfortable and as I needed yet another script to migrate docker package images from the aswftesting to the aswf organisation I thought I would try python...

So I've replaced around 1k lines of untestable bash+hcl configuration files with around 1k lines of statically typed python 3 with around 96% test coverage (there are 500 lines in this PR coming from the Pipfile.lock file which "locks" in place current dependencies to avoid random breakages when new releases happen).

I have described the new python utilities there: https://github.com/aloysbaillet/aswf-docker/blob/add-migrate-script/python/README.md
The python code is all formatted using black, statically typed-tested with mypy, tested with pytest and PEP8-tested with prospector, with pre-commit support to enforce all of this automatically before commiting code, and added corresponding Azure Pipeline tests. Code coverage is also reported in a badge on that python README file.

New utilities:

  • aswfdocker packages lists all ci packages.
  • aswfdocker images lists all ci images.
  • aswfdocker migrate allows migration of docker packages from one organisation to another.
  • aswfdocker build builds docker ci packages and ci images.
  • aswfdocker getdockerorg prints the current dockerhub organisation to use.
  • aswfdocker getdockerpush prints if the images should be pushed.

Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
from aswfdocker.cli import aswfdocker


class TestBuilder(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like test_builder.py is hard coded to use 2019 as the test case, should that be configurable so it can easily be moved forward / backwards, or even across all supported years if you wanted to be completely thorough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking about this a bit and the added complexity scares me more than having to update some tests in 2 years time...

@jfpanisset
Copy link
Contributor

All looks great to me, none of the comments I made are blockers, so if you want I'm happy to go ahead and give final approval so this can go in and start to get used.

Also generally it seems you've put in place some interesting infrastructure for Python code that could deserve to be propagated across ASWF projects. Maybe worth writing up a short summary for the TAC list, and could be incorporated in the ASWF sample project documents.

jfpanisset
jfpanisset previously approved these changes Feb 18, 2020
Copy link
Contributor

@jfpanisset jfpanisset left a comment

Choose a reason for hiding this comment

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

Approved

@aloysbaillet
Copy link
Contributor Author

Thanks for the review JF! I think I've addressed most of your comments.

jfpanisset
jfpanisset previously approved these changes Feb 19, 2020
Copy link
Contributor

@jfpanisset jfpanisset left a comment

Choose a reason for hiding this comment

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

All looks good.

@aloysbaillet
Copy link
Contributor Author

There was a merge conflict which means I had to push another commit which discarded your approval... again...
I also just added automatic tagging on build, but these are not pushed yet during automation.

Copy link
Contributor

@jfpanisset jfpanisset left a comment

Choose a reason for hiding this comment

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

All looks good!

@aloysbaillet aloysbaillet merged commit cff3ff6 into AcademySoftwareFoundation:master Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants