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

feat: add common workflow for taps and targets #33

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
39 changes: 39 additions & 0 deletions .github/workflows/common/common_pytest.yml
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]
Copy link

@WillDaSilva WillDaSilva Jan 24, 2023

Choose a reason for hiding this comment

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

Suggested change
on: [push]
on:
pull_request: {}
push:
branches: [main]
workflow_dispatch:
inputs: {}
# Cancel in-progress jobs if a newer job supersedes it
concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true

Copy link
Author

@aaronsteers aaronsteers Jan 24, 2023

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.

  • Workflow dispatches to allow these to be run manually - I think that makes sense.
  • What's the benefit of pull_request: {}, push: { branches: [main] } vs just on: [push]?
  • I could guess but I'm not confident I know what the concurrency block does. An inline comment would be helpful for clarity+maintainability I think.

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 when main is pushed to. It also support running the workflow manually, as you said.

The concurrency block groups running jobs if they have the same group, which is ${{ github.workflow }}-${{ github.head_ref || github.run_id }}, e.g. common_pytest-1234-branch-name, or common_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.

Copy link
Author

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.

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?

Copy link
Member

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.

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 (plus workflow_dispatch) if you think running on every push is valuable.

Copy link
Author

@aaronsteers aaronsteers Jan 24, 2023

Choose a reason for hiding this comment

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

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.

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.


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

37 changes: 37 additions & 0 deletions .github/workflows/common/common_python_tox_lint.yml
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.

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

@WillDaSilva Is there an alternative you prefer?

Choose a reason for hiding this comment

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

For linting? I've been finding pre-commit pretty good for that job, but we don't need a workflow for that since we could use https://pre-commit.ci instead.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Author

@aaronsteers aaronsteers Jan 24, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

@aaronsteers aaronsteers Jan 24, 2023

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The 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?

pre-commit run --all-files

n.b. the default pre-commit / pre-commit run command will only run on the staged changes.

Choose a reason for hiding this comment

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

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?

Make sense 👍

# 2. The lint operation will be run on the below-named python version.

name: Python Lint Checks from Tox (MeltanoLabs)

on: [push]
Copy link

@WillDaSilva WillDaSilva Jan 24, 2023

Choose a reason for hiding this comment

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

Suggested change
on: [push]
on:
pull_request: {}
push:
branches: [main]
workflow_dispatch:
inputs: {}
# Cancel in-progress jobs if a newer job supersedes it
concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true


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
61 changes: 61 additions & 0 deletions .github/workflows/common/common_release_workflow.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
name: Publish GitHub Release to PyPI (MeltanoLabs)

# 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
# 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 }}"
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Choose a reason for hiding this comment

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

Automated Release Notes drafting using Semantic PRs

Do you intend to add a workflow for this before merging this PR?

Choose a reason for hiding this comment

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

Copy link
Author

@aaronsteers aaronsteers Jan 24, 2023

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

Copy link
Author

@aaronsteers aaronsteers Jan 24, 2023

Choose a reason for hiding this comment

The 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 main, so that each main update automatically updates/creates the release notes draft.

As such, probably this should wait for a subsequent PR.

Choose a reason for hiding this comment

The 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.