Skip to content

Conversation

@amannocci
Copy link
Contributor

What is the change being made?

How has this been tested?

Related Issues

Closes #1835

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci amannocci requested review from a team, basepi and beniwohli July 10, 2023 12:57
@amannocci amannocci self-assigned this Jul 10, 2023
@amannocci amannocci requested review from a team and removed request for a team July 10, 2023 13:08
Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

Just a note about generating the source distribution (sdist), otherwise LGTM!

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

I have the same concern as @beniwohli but otherwise this looks good.

runs-on: ubuntu-latest
env:
PYPI_SECRET_PATH: secret/apm-team/ci/apm-agent-python-pypi-prod
environment: release
Copy link
Member

Choose a reason for hiding this comment

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

Q: What is the purpose of the environment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A: It allows to apply specific restriction about who trigger the workflow and is allowed to publish an artifact.

Configuring an environment is optional, but strongly recommended: with a GitHub environment, you can apply additional restrictions to your trusted workflow, such as requiring manual approval on each run by a trusted subset of repository maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

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

The security applied by using environments is a side effect. I would not recommend using that approach.

Copy link
Member

Choose a reason for hiding this comment

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

The security applied by using environments is a side effect. I would not recommend using that approach.

From what I read here, it's designed for this usecase https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment.

I was just surprised to see no protection rules as checked in the environment settings. Hence, I did not understand why this was created.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is designed to approve deployments, not workflows; it is not strictly the same.

If you need approval to release something, maybe the trigger is not the creation of a tag. Maybe those tags should be created by the CI when you bump the version o a file, or you make another change in the repository that will require approval of a PR and will be triggered when you merge the PR.

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Copy link
Contributor

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

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

LGTM!

@amannocci amannocci merged commit 643afa2 into main Jul 12, 2023
@amannocci amannocci deleted the feat/trusted-publisher branch July 12, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch to trusted publishers on PyPI

6 participants