-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add common workflow for taps and targets #33
base: main
Are you sure you want to change the base?
Changes from 7 commits
8cc87a7
93ab7ae
e777ecd
554d832
0e31404
82e86db
4c9fdc4
e23f73c
717f043
78a97c2
57d45fc
fe887b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# A common CI workflow that runs pytest on all actively supported Python versions. | ||
# | ||
# Assumptions: | ||
# | ||
# 1. The repo works with the below-specified version of Poetry. | ||
# 2. The repo can be tested with pytest. | ||
# 3. All Python versions named below are enabled for testing. | ||
|
||
name: Python Pytest Check (MeltanoLabs) | ||
|
||
on: [push] | ||
|
||
jobs: | ||
pytest: | ||
runs-on: ubuntu-latest | ||
env: | ||
GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}} | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
python-version: [3.7, 3.8, 3.9, 3.10] | ||
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Install Poetry | ||
uses: snok/install-poetry@v1 | ||
with: | ||
version: 1.3.2 | ||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v4 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
cache: 'poetry' | ||
- name: Install dependencies | ||
run: | | ||
poetry install | ||
- name: Test with pytest | ||
run: | | ||
poetry run pytest --capture=no | ||
aaronsteers marked this conversation as resolved.
Show resolved
Hide resolved
WillDaSilva marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,37 @@ | ||||||||||||||||||||||||||
# A common CI workflow to run linting as defined in tox.ini | ||||||||||||||||||||||||||
# | ||||||||||||||||||||||||||
# Assumptions: | ||||||||||||||||||||||||||
# | ||||||||||||||||||||||||||
# 1. The repo has a tox.ini file, which defines a 'lint' command. | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to be promoting Tox? It's a rather painful tool in my experience. Most of the time when I run it for the first time in a new repository it fails in some obscure way because my Python installation doesn't match that of the one who set up Tox for the repository. That's not really Tox's fault, it's more of an indictment against Python packaging and version management as a whole, but it's still a poor experience for contributors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WillDaSilva Is there an alternative you prefer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For linting? I've been finding In either case, you need to ensure you're running on the right Python version, whatever that may be for a given project. Here we assume that's Python 3.10. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I'm 100% in favor of replacing Tox with pre-commit for all lint checks & fixes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love not having a lint job defined in CI, but that's a separate matter. Tox is the only thing we had that could run all 4 or 5 lint tools from a single comment, the same way on local as in the CI cloud service. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's a way to manually invoke the equivalent of what the cloud runs with pre-commit.ci, yes? I'm a bit behind on that invocation, and might need to do a deeper dive / refresher. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, I think I'm in favor of committing this lint workflow since it pairs with our cookiecutter. I've already named the file in a way that it is specific to this invocation pattern. We can just not enable it for repos where it isn't useful, but in the meanwhile, we still have the option to do so for repos where the tox-based lint rules exist and the pre-commit.ci set does not (yet). What do you think of that, at least for short-term? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
n.b. the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Make sense 👍 |
||||||||||||||||||||||||||
# 2. The lint operation will be run on the below-named python version. | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
name: Python Lint Checks from Tox (MeltanoLabs) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
on: [push] | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
jobs: | ||||||||||||||||||||||||||
linting: | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
runs-on: ubuntu-latest | ||||||||||||||||||||||||||
strategy: | ||||||||||||||||||||||||||
matrix: | ||||||||||||||||||||||||||
# Only lint using the primary version used for dev | ||||||||||||||||||||||||||
python-version: [3.10] | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
steps: | ||||||||||||||||||||||||||
- uses: actions/checkout@v3 | ||||||||||||||||||||||||||
- name: Install Poetry | ||||||||||||||||||||||||||
uses: snok/install-poetry@v1 | ||||||||||||||||||||||||||
with: | ||||||||||||||||||||||||||
version: 1.3.2 | ||||||||||||||||||||||||||
- name: Set up Python ${{ matrix.python-version }} | ||||||||||||||||||||||||||
uses: actions/setup-python@v4 | ||||||||||||||||||||||||||
with: | ||||||||||||||||||||||||||
python-version: ${{ matrix.python-version }} | ||||||||||||||||||||||||||
cache: 'poetry' | ||||||||||||||||||||||||||
- name: Install dependencies | ||||||||||||||||||||||||||
run: | | ||||||||||||||||||||||||||
poetry install | ||||||||||||||||||||||||||
- name: Run lint command from tox.ini | ||||||||||||||||||||||||||
run: | | ||||||||||||||||||||||||||
poetry run tox -e lint |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
name: Publish GitHub Release to PyPi (MeltanoLabs) | ||
aaronsteers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# This workflow executes upon the publishing of a release in GitHub. This does not run on draft releases. | ||
# | ||
# Assumptions: | ||
# | ||
# 1. The repo is a python project which can be deployed using the Python version below noted in `python-version: __`. | ||
# 2. The `pyproject.toml` file should declare `version = 0.0.0` in deference of dynamic versioning. | ||
# 3. The release title should be in the semvar format (`vX.Y.Z`) | ||
# 4. The repo secrets should declare (or have access to) the following env vars / secrets: | ||
# 1. TEST_PYPI_TOKEN - A token to use for test publish. | ||
# 2. PYPI_TOKEN - A token to use for actual publish. | ||
# | ||
# Other notes: | ||
# | ||
# - Unless you block the hotkey, you browser will likely interpret "Ctrl+Enter" as "Publish" and not "Save Draft", | ||
# even if you are editing an existing draft. | ||
# - The text copy of Release Notes can freely be edited after publish, with no impact on the publish process and without | ||
# risk of accidentally republishing. | ||
# - PyPi will only accept each semvar increment once, but failures to publish can be freely retried, and you can yank | ||
aaronsteers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# and release a patch on top if needed. | ||
|
||
on: | ||
release: | ||
types: [published] | ||
|
||
jobs: | ||
build_deploy: | ||
|
||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
with: | ||
fetch-depth: 0 | ||
- name: Set up Python | ||
uses: actions/setup-python@v3 | ||
with: | ||
python-version: '3.10' | ||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install poetry | ||
- name: Build package | ||
run: | | ||
poetry self add "poetry-dynamic-versioning[plugin]" | ||
poetry config repositories.testpypi https://test.pypi.org/legacy/ | ||
poetry dynamic-versioning --no-cache | ||
poetry build | ||
- name: Upload wheel to release | ||
uses: svenstaro/upload-release-action@v2 | ||
with: | ||
repo_token: ${{ secrets.GITHUB_TOKEN }} | ||
file: dist/*.whl | ||
tag: ${{ github.ref }} | ||
overwrite: true | ||
file_glob: true | ||
- name: Deploy to PyPI | ||
run: | | ||
poetry publish -r testpypi -u "__token__" -p "${{ secrets.TEST_PYPI_TOKEN }}" | ||
poetry publish -u "__token__" -p "${{ secrets.PYPI_TOKEN }}" | ||
aaronsteers marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -148,3 +148,18 @@ Check out all the [published connectors](https://hub.meltano.com/singer/maintain | |
| [target-yaml](https://github.com/MeltanoLabs/target-yaml) | Prerelease (Beta) | 🔭 Looking For Maintainers! 🔭 | [Link](https://hub.meltano.com/targets/yaml--meltanolabs) | | ||
| [target-snowflake](https://github.com/MeltanoLabs/target-snowflake) | Prerelease (Beta) | [Meltano](https://github.com/meltano) - Maintainer | - | | ||
| [target-jsonl-blob](https://github.com/MeltanoLabs/target-jsonl-blob) | Prerelease (Beta) | [Edgar Ramírez](https://github.com/edgarrmondragon) - Maintainer | - | | ||
|
||
## Common CI Workflows | ||
|
||
MeltanoLabs taps and targets can share workflows via the GitHub [Required Workflows](https://github.blog/2023-01-10-introducing-required-workflows-and-configuration-variables-to-github-actions/#:~:text=Required%20workflows%20allows%20DevOps%20teams,impossible%20task%20in%20large%20organizations.) feature. | ||
|
||
Repositories are "opted in" to shared workflows via the [Add Required Workflows](https://github.com/organizations/MeltanoLabs/settings/actions/required_workflows/new) GitHub web UI. | ||
|
||
When eligible for this behavior, and when the repo is opted-in by a MeltanoLabs administrator, common CI workflows may include (but are not necessarily limited to): | ||
|
||
1. Automated CI testing on currently-supported Python versions. | ||
1. Automated verification of Semantic PR logic. | ||
1. Release management using dynamic versioning. | ||
1. Automated Release Notes drafting using Semantic PRs. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you intend to add a workflow for this before merging this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is how we do it currently for Meltano and the SDK: https://github.com/meltano/sdk/blob/1ee4c4ab32bdb3b7c7e88711df2e455a2826c6ed/.github/workflows/version_bump.yml#L52-L74 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WillDaSilva - If it is ready to go, we can certainly add it. I wasn't planning on blocking for it though, since we can add/iterate afterwards too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aaronsteers I just don't think it should be documented if it isn't present. I agree that we can add it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second review, I think as the workflow is defined today, it must be triggered manually. We probably would want to make this idempotent (if not already so) and likely trigger on all pushes to As such, probably this should wait for a subsequent PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
For a full list of common workflows, see the `.github/workflows` directory within this repo. Each workflow should be self-documenting, including assumptions and behavior notes. |
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.
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.
I don't know if I follow the distinctions here.
pull_request: {}, push: { branches: [main] }
vs juston: [push]
?concurrency
block does. An inline comment would be helpful for clarity+maintainability I think.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.
on: push
triggers a workflow run for each push, even if there is no associated PR.The suggested
on
block triggers a workflow run for each update to the PR (e.g. each time new commits are pushed), and will additionally run the workflow whenmain
is pushed to. It also support running the workflow manually, as you said.The
concurrency
block groups running jobs if they have the samegroup
, which is${{ github.workflow }}-${{ github.head_ref || github.run_id }}
, e.g.common_pytest-1234-branch-name
, orcommon_pytest-12341251346132346
. When a new job named ABC is started that matches that group, and an old job ABC within the group is still in-progress, the old one is cancelled. This is so that if someone pushes multiple times to a PR in quick succession, only the most recent jobs will run to completion, since the old ones are obsolete. We use this same block of code in several Meltano repositories.I'll update it with a comment explaining it.
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.
I think this was my desired behavior, to run on all commits even if a PR doesn't (yet) exist and including
main
branch along with all other branches. Any reason not to run if a PR doesn't exist yet?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.
FWIW this may trigger duplicate runs once the PR is created, one for the push and another for the PR sync.
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.
There isn't much visibility when automatically running workflows without a PR, and it may waste compute (which we don't pay for for public repos, but it still feels wasteful).
I have no strong objection to using
on: push
(plusworkflow_dispatch
) if you think running on every push is valuable.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.
Okay, this makes sense. I don't personally mind those jobs running to completion for taps/targets where the duration is rather short. In the past I have pushed a commit followed by a second one and maybe a third, specifically to see how the variations will perform in CI, essentially parallelizing the work by offloading it.
I'm happy to use the dedupe / short-circuit logic you propose. I slightly prefer not to dedupe, but I do not feel strongly enough to push back if that's your preferred behavior.