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

Add a PyPI publishing GitHub Actions CD workflow #79469

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Nov 25, 2022

SUMMARY

This patch allows productization folks cut releases using a Run workflow form that will be located at https://github.com/ansible/ansible/actions/workflows/build-and-publish.yml.

Demo for the invalid version entered: https://github.com/webknjaz/ansible/actions/runs/3550849493#summary-9714757781.
Demo for the post-publish job: https://github.com/webknjaz/ansible/actions/runs/3550719797#summary-9714495335 (created from a modified workflow with an if: always() clause).

ISSUE TYPE
  • Packaging Pull Request
COMPONENT NAME

.github/workflows/build-and-publish.yml

ADDITIONAL INFORMATION

This attempts to replace #79453.
Refer to the internal Slack thread for the initial discussion.

This PR includes the #79468 patch which can be merged separately, right away.

@ansibot ansibot added affects_2.15 needs_triage Needs a first human triage before being processed. labels Nov 25, 2022
@sivel
Copy link
Member

sivel commented Nov 28, 2022

Somethings to note here:

  1. We need a make docs to run before we run python -m build
  2. Why are we only building an sdist, and not also building a wheel?

@webknjaz webknjaz force-pushed the packaging/gha-workflow-publish-to-pypi branch from 3731c17 to 1a3840c Compare November 28, 2022 17:52
@webknjaz
Copy link
Member Author

Somethings to note here:

1. We need a `make docs` to run before we run `python -m build`

What does it do? Does it generate RST files to be committed?
FWIW @rcarrillocruz pointed me at https://ansible.pages.redhat.com/productization/packaging_and_delivery/ansible-core-release.html which does not have such an instruction.

If it is supposed to only be included in source distributions, this begs a question of why isn't it implemented though an in-tree PEP 517 build backend wrapper.

2. Why are we only building an sdist, and not also building a wheel?

Last time I asked, there were still symlinks we wanted to retain. I thought such a change was supposed to go through fallible first. But if it's not the case anymore, it's a good idea to ship wheels too.

@jborean93
Copy link
Contributor

What does it do? Does it generate RST files to be committed?

I assume the make docs is not included in the Python build steps because it's an unnecessary step for local builds and adding it would increase how long it would take to create the sdist but that's just an assumption.

Last time I asked, there were still symlinks we wanted to retain. I thought such a change was supposed to go through fallible first. But if it's not the case anymore, it's a good idea to ship wheels too.

We've shipped wheels since 2.13.x - https://pypi.org/project/ansible-core/2.13.0/#files.

@sivel
Copy link
Member

sivel commented Nov 28, 2022

What does it do? Does it generate RST files to be committed?

It generates the man pages. Why we don't do it another way I don't know. We used to run make sdist which is no longer the case, and it handled the docs target first. When we moved to using build instead, we just added a make docs before that step to the jenkins job.

See #77820

Last time I asked, there were still symlinks we wanted to retain...

As @jborean93 states, that hasn't been the case since we revamped the installation for 2.13.

@rcarrillocruz
Copy link
Contributor

After the package has been built, published and the commit is tagged there is a post step to move
the lib/ansible/release.py to post0.
I'd add that to the post release activities. Typical PR for that is as follows:

#77130

@webknjaz webknjaz force-pushed the packaging/gha-workflow-publish-to-pypi branch from 1a3840c to 57a4189 Compare November 29, 2022 11:44
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Nov 29, 2022
@webknjaz
Copy link
Member Author

What does it do? Does it generate RST files to be committed?

I assume the make docs is not included in the Python build steps because it's an unnecessary step for local builds and adding it would increase how long it would take to create the sdist but that's just an assumption.

Even more reasons to explore having an in-tree build backend (supported since pip v20.0 and Python 3.9 bundles pip v20.2.1+).
PEP 517 allows passing arbitrary args to the backend via config_settings which effectively means that we can have it as an opt-in via something like python -m build --config-setting='--build-manpages'.

Last time I asked, there were still symlinks we wanted to retain. I thought such a change was supposed to go through fallible first. But if it's not the case anymore, it's a good idea to ship wheels too.

We've shipped wheels since 2.13.x - pypi.org/project/ansible-core/2.13.0/#files.

Ah, I didn't realize. Good to know! So this effectively means that https://ansible.pages.redhat.com/productization/packaging_and_delivery/ansible-core-release.html is incomplete indeed.

What does it do? Does it generate RST files to be committed?

It generates the man pages. Why we don't do it another way I don't know. We used to run make sdist which is no longer the case, and it handled the docs target first. When we moved to using build instead, we just added a make docs before that step to the jenkins job.

See #77820

FWIW this step isn't mentioned on the page the productization team uses. I think I'll look into having a PEP 517 in-tree wrapper backend — I've got experience and deep understanding of doing so in pylibssh already.

After the package has been built, published and the commit is tagged there is a post step to move the lib/ansible/release.py to post0. I'd add that to the post release activities. Typical PR for that is as follows:

#77130

That's already incorporated in the PR (kinda).

- name: ⚙️ Set the target Git 🏷️ tag
id: git
run: |
echo 'release-branch=release/${{
Copy link
Member Author

Choose a reason for hiding this comment

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

@mkrizek if we can make ansibot allow PRs from in-upstream release branches, I would also make this workflow create PRs, not just a branch. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

ansible/ansibullbot#971 from @mattclay is the original issue for the bot requesting to close PRs from an upstream branch and cancel the CI run for it. I am not sure we want to re-allow such PRs though I don't remember why we disallowed them in the first place :-)

I suppose at the very least if we wanted to we could make an exception for PRs from this workflow (based on criteria like PR submitter and/or branch name, $other) 🤷🏻‍♂️

Copy link
Member Author

@webknjaz webknjaz Nov 30, 2022

Choose a reason for hiding this comment

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

ansible/ansibullbot#971 from @mattclay is the original issue for the bot requesting to close PRs from an upstream branch and cancel the CI run for it. I am not sure we want to re-allow such PRs though I don't remember why we disallowed them in the first place :-)

This is because when the upstream is populated with random branches, the end-users will get them copied in forks. Depending on the time, different users would have a copy of such branches in different states, and they won't be cleaned up from the forks once unnecessary. Plus, many branches in forks would create confusion too. So in general, it's a good idea to keep the upstream clean, except for special cases such as this one.

I suppose at the very least if we wanted to we could make an exception for PRs from this workflow (based on criteria like PR submitter and/or branch name, $other) 🤷🏻‍♂️

Yes, that's exactly what I'm asking for — making an exception for certain PR. Either by user.type == 'Bot' or by some label. The bot check would work for any GitHub App (if we ever have one creating PRs) and the label could take care of the human-like bots. The branch pattern could work but would require re-hardcoding every time with add some other automation needing this.

Copy link
Member

Choose a reason for hiding this comment

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

There were two reasons for not wanting upstream branches:

  1. Branch pollution, as @webknjaz mentioned.
  2. Duplicate CI runs for PRs (one from the branch, another from the PR). This is no longer an issue, now that we're using AZP and have configured it to only test specific branches.

@webknjaz Is it not possible for the workflow to create branches in a user's fork instead?

Copy link
Member Author

@webknjaz webknjaz Dec 2, 2022

Choose a reason for hiding this comment

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

@mattclay the auto-provisioned GITHUB_TOKEN secret is very limited in what it can do. For example, when it mutates anything on the platform, the corresponding events don't propagate to any listeners (neither other apps/integrations, not GHA itself, including other job definitions).

But yes, it is possible with extra maintenance burden being the following:

  1. a (dedicated?) user/org with a fork
  2. somebody with full repo access will have to maintain a secret with an API key for accessing another user resources — IIRC they now have a mandatory expiration date
  3. there needs to be a guarantee that the fork's branches aren't going to be overridden

I suppose, it could be easier to have GitHub App credentials because of (2). We'd still need to have a dedicated forked repository, maybe under one of our orgs like community it'd work. That App would need to be installed here and in the fork (this bit is easy). There's no mandatory need to actually run a web server for the app listening to the events — the secrets could be just used in GHA directly for getting a qualifying token.

This is one of the Patchback's annoyances — many people do want backport PRs from forks. But then, it's harder to maintain such a system. This is mostly solved by enabling the Automatically delete head branches checkbox in the repository settings, which lets GitHub auto-clean the corresponding branches post-merge.

Now that I think about it more, it would definitely be useful to have a GHA regardless of whether we'll accept the burden of mangling with forks.

P.S. If we were to have a more canonical type of GitHub App with a web-app interface, we could authenticate users and just use their forks w/o having a dedicated one. But that would be substantially more work, and it's probably not worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

when it mutates anything on the platform, the corresponding events don't propagate to any listeners

This is why the current implementation doesn't attempt to auto-create a PR.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 10, 2022
@webknjaz webknjaz marked this pull request as draft December 12, 2022 16:03
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Dec 12, 2022
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jan 16, 2024
@ansibot ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jan 16, 2024
@webknjaz webknjaz added the unimportant_ci This PR does not need to have healthy CI status and should be ignored by the CI infra maintainers. label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.15 ci_verified Changes made in this PR are causing tests to fail. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. unimportant_ci This PR does not need to have healthy CI status and should be ignored by the CI infra maintainers. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants