Skip to content

Conversation

@GabDug
Copy link
Collaborator

@GabDug GabDug commented Feb 10, 2024

Hey! Quick MR, mostly about packaging, CI and a few Python simplifications. Non exhaustive overview:

CI

  • Added dependant for the Github Actions versions (weekly)
  • Added general ci.yml
    • On PR/commit on main, run tests, linters and build package. Run on multiple Python versions, only on AMD64 ubuntu latest (we can add a bigger OS/arch matrix if needed)
    • For the moment, only builds the packages, but I can add the linters / tests of your choice
  • Added release.yml
    • Triggered by a manual workflow, allows to trigger for a specific commit, create the associated Git tag
      • This is imo the most powerful setup, but we can adapt it to trigger upon a tag, or upon a Github release creation.
    • Publish to Pypi
    • Create a Github Release, included the Python artefacts (whl and `tar.gz)
    • Create an ARM64 and AMD64 image with the package
  • Added sync_python_deps.yml
    • PDM is not covered by dependabot or renovate, hence a specific workflow to update the lockfile (and associated pre-commits)

Packaging

  • Added pyproject.toml
  • Added license
  • Move source files (and payloads) to src
    • Modern Python package!

README

  • Added Links to Pypi as badges, with latest versions and supported Python versions
  • Changed usage from ./bypass-url-parser to bypass-url-parser

Further improvements

  • Symlink the moved files?
  • Add a CONTRIBUTING.md guide for newcomers / maintainers?
  • Add pre-cmmot.ci?

@laluka laluka marked this pull request as ready for review February 11, 2024 15:13
Copy link
Owner

@laluka laluka left a comment

Choose a reason for hiding this comment

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

Lovely work, tons of new things I'm not too familiar with yet (gitlab CI fan, gha is somewhat different..)!
A few questions, a few neats, maybe some "onboarding time" for the two of us, and another approval from @jtof-fap and we'll merge this one for sure, thanks for the hard/smart work! 💟

with:
cache: true
python-version: ${{ matrix.python-version }} # Version range or exact version of a Python version to use, the same as actions/setup-python
architecture: x64 # The target architecture (x86, x64) of the Python interpreter. the same as actions/setup-python
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add x86 & arm 32/64 from the first release, for mac MX users ? 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need it, as our code is pure Python it is universal. We could expand the architecture/OS matrix if we want, but as there are no tests yet it would not be useful.
The Docker image is arch specific, but already has amd64 and arm64 as targets.

python-version: ${{ matrix.python-version }} # Version range or exact version of a Python version to use, the same as actions/setup-python
architecture: x64 # The target architecture (x86, x64) of the Python interpreter. the same as actions/setup-python
version: ${{ matrix.pdm-version }} # The version of PDM to install. Leave it as empty to use the latest version from PyPI, or 'head' to use the latest version from GitHub
allow-python-prereleases: true # Allow prerelease versions of Python to be installed. For example if only 3.12-dev is available, 3.12 will fall back to 3.12-dev
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe false to only drive efforts to true releases ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As you want, I saw that the Docker image was using 3.13, so I thought it was a good idea!

Building and testing against future versions of Python should not be a huge issue, as there are not many dependencies and it's a pure Python package.

workflow_dispatch:
inputs:
tag:
description: "The version to tag, without the leading 'v'. If omitted, will initiate a dry run (no uploads)."
Copy link
Owner

Choose a reason for hiding this comment

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

So we buildon every push, and deploy release on specific tag vX.Y.Z ? Any keyword to specify ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, we have a CI for each commit/PR, and the release one on manual trigger.

Just have to trigger the CD action with the tag to create e.g. 0.4.0 or 0.4.0alpha1 and it will deploy to Pypi, Github Release, create the Git tag, and push a Docker image to Github Registry.

@@ -1,5 +1,7 @@
# Bypass Url Parser

[![PyPI - Version](https://img.shields.io/pypi/v/bypass-url-parser)](https://pypi.org/project/bypass-url-parser/) [![PyPI - Python Version](https://img.shields.io/pypi/pyversions/bypass-url-parser)](https://pypi.org/project/bypass-url-parser/) [![PyPI - License](https://img.shields.io/pypi/l/bypass-url-parser)](https://github.com/laluka/bypass-url-parser/blob/main/LICENSE) [![pdm-managed](https://img.shields.io/badge/pdm-managed-blueviolet)](https://pdm.fming.dev)
Copy link
Owner

Choose a reason for hiding this comment

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

🌹

]

[[tool.pdm.autoexport]]
filename = "requirements.txt"
Copy link
Owner

Choose a reason for hiding this comment

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

Auto-export, so should we remove from git & add to gitignore requirements*.txt ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added it in VCS to allow users without PDM to install or edit the package with a simple clone. If you feel that's too much, I'd be glad to removed the requirements*.txt and the PDM plugin.

if self.save_level >= self.SaveLevel.PERTINENT:
json_file = f"{outdir}{Tools.SEPARATOR}{Bypasser.DEFAULT_JSON_FILENAME}"
with open(json_file, mode="wt", encoding=self.encoding) as file:
with open(json_file, mode="w", encoding=self.encoding) as file:
Copy link
Owner

Choose a reason for hiding this comment

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

We'll have to check that this doesn't introduct breaking changes.. 😅
And test that though installs from pdm, pip, pipx.. X_X

Copy link
Collaborator

Choose a reason for hiding this comment

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

The t doesn't get in the way but it's not essential here either, it's the default behaviour :

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, all changes in this file should be safe, they were produced by ruff, sorry for the noise 😅

Only manual changes are in load_file_into_memory_list

@laluka
Copy link
Owner

laluka commented Feb 25, 2024

In call with @GabDug , merging now to troubleshoot live if needed :)

1 similar comment
@laluka
Copy link
Owner

laluka commented Feb 25, 2024

In call with @GabDug , merging now to troubleshoot live if needed :)

@laluka laluka merged commit 7e38a77 into laluka:main Feb 25, 2024
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.

3 participants