-
Notifications
You must be signed in to change notification settings - Fork 52
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
π· Create lint and test workflow #63
Conversation
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
β¦er to upgrade to python 3.9 Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Someone else can figure it out later :-) Signed-off-by: ff137 <ff137@proton.me>
@esune, Assigning you and as reviewers since I think we should both take a look. Assigned to me to get it on my backlog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes overall look good to me, just a question regarding a potentially unused dependency.
python3_indy~=1.16.0.post286 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick scan, this doesn't seem to be imported anywhere - is it a transitive dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@esune import indy
is used in the integration tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be great if there was an alternative way to achieve that integration testing, because figuring out how to install the libindy dependency on the docker image probably took me a good 2 hours of trial and error π
That's why Dockerfile.test now builds from ubuntu:20.04 and installs python separately, because that apt-get install libindy
doesn't work on debian images, and python docker images all use debian, AFAICT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should migrate to indy-vdr
at some point in the near future.
π· Creates a lint-and-test.yaml github workflow, to automate testing and enforce some code style checks
π· Add Dependabot pip dependency management config
β add black and isort dev dependencies
π pin dependency versions for best practice
π¨ apply black and isort formatting to python files
β¬οΈ Upgraded test dockerfile base image to use ubuntu:20, so that python can be upgraded to 3.9. Previous base image used 3.6, and ubuntu base image was chosen because it's easier to install the libindy dependency on ubuntu than debian, which official python base images run on.
(Side note: no offense, but it's kinda bad to be running Python 3.6 in 2024 -- it's been end-of-life since Dec 2021! And it's not a heavy lift to update to the new python versions - nevertheless it will be a process to make that migration across the board)