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

Worklow to publish a package from a CODEOWNER tag #598

Merged
merged 6 commits into from
Aug 19, 2021

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Jul 21, 2021

Description

Adds a new workflow (.github/publish-a-package.yml) and a new build script (scripts/build_a_package.sh) to allow releases of contrib repo packages from a push of a tag.

The hope is that once we approve CODEOWNERS who own contrib packages, they can release packages independent of the maintainers by simply pushing a tag with a package name and package version that matches a package in this repo.

The format of the tag should be:

opentelemetry-<PKG_NAME>==<PKG_VERSION>

NOTE: You can only release a package that is >=1.0! This is to allow beta packages to be released by the regular release workflow when the maintainers have a chance to do it.

I also updated the original script to skip .tar.gz files which are >=1.0 for this very reason.

This is very similar to the setup over on opentelemtry-dotnet-contrib where they have a separate workflow for every package. That workflow gets triggered on a push to a relevant tag. However, my goal here is to one have one workflow for each package.

Also see the .NET PR that added this functionality: open-telemetry/opentelemetry-dotnet-contrib#64

The PRO is less code duplication.

The CON is less control by CODEOWNERS as to what the workflow does for their specific package (although we can address those later on a case-by-case need)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
    - [ ] This change requires a documentation update No?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • You can run ./scripts/build_a_package.sh locally, you just need to:

    • Set GITHUB_REF= to something like refs/tags/opentelemetry-sdk-extension-aws==0.23b0
    • Update the BASE_DIR variable to the location of your contrib repo
  • To test the actual workflow, I'll try to create my own repo sometime this week to validate everything works.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
    - [ ] Changelogs have been updated
    - [ ] Unit tests have been added Didn't see any for the previous release flow?
    - [ ] Documentation has been updated

@NathanielRN NathanielRN requested review from a team, ocelotl and lzchen and removed request for a team July 21, 2021 01:06

jobs:
publish:
name: Publish package from tag ${{ env.GITHUB_REF }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable is crucial to make this whole setup work. The variable is defined on GitHub's docs and I took inspiration from this SO post on how to get the currently pushed tag in GitHub actions for the rest of the PR.

@aabmass
Copy link
Member

aabmass commented Jul 21, 2021

Nice this is pretty neat

Adds a new workflow (.github/publish-a-package.yml) and a new build script (scripts/build_a_package.sh) to allow releases of contrib repo packages from a push of a tag.

How would a CODEOWNER push to a tag? Wouldn't this still require the CODEOWNER to have push access?

@NathanielRN
Copy link
Contributor Author

@aabmass Thanks for the review! It prompted me to test the results out on my own repo and validate that it works 🙂

Here is a workflow where the publish fails because the tag package name does not correspond to any package.

Here is another workflow where the publish fails because the tag matches a package but does not match the current version of the package in the repo.

Let me know if you have feedback on these?

@NathanielRN NathanielRN requested a review from aabmass July 21, 2021 19:05
@NathanielRN NathanielRN requested a review from aabmass July 21, 2021 21:40
exit -1
fi

PKG_NAME_AND_VERSION=${GITHUB_REF#refs/tags/*}
Copy link
Member

Choose a reason for hiding this comment

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

One other bash nit, i'd use lowercase variable names for non-environment variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea! I was following ./scripts/build.sh but we should probably update that one instead. Updated!

@NathanielRN NathanielRN requested a review from aabmass July 23, 2021 03:46
@@ -0,0 +1,68 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Not asking you to translate this, but maybe we should consider writing these tools in Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we definitely could! I just wrote it like this for now so people could compare it against the ./build.sh in the same repo and see the difference 🙂 I think it's a small enough script to change to Python in the near future if we want!

@aabmass
Copy link
Member

aabmass commented Aug 2, 2021

Code LGTM but I'm not sure if we've agreed on the approach enough to merge yet. My understanding is this isn't needed for a while until that checklist is complete, right?

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 5, 2021
@NathanielRN NathanielRN force-pushed the separate-package-releases branch from 4729936 to 9b24978 Compare August 5, 2021 16:07
@lzchen
Copy link
Contributor

lzchen commented Aug 5, 2021

LGTM.

@codeboten
Thoughts on this?

@NathanielRN NathanielRN closed this Aug 6, 2021
@NathanielRN NathanielRN reopened this Aug 6, 2021
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Would suggest to run a test with a test/local pypi server and a fake package.

run: |
pip install twine
# We don't need to publish to TestPyPI because we only publish 1 package.
# If it fails no other work needs to be reversed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is fine but I think we should add this and then verify the package by installing it with pip. Once everything works as expected, only then we publish to real pypi service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not asking we do it in this PR. Can we create a ticket for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I made #622 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants