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

Use private runners & GitHub app token for Release jobs #252

Closed
wants to merge 2 commits into from

Conversation

mars
Copy link
Member

@mars mars commented Sep 5, 2024

I'm trying to use this workflow with a private repo, and hit some snags.

@mars mars requested a review from a team as a code owner September 5, 2024 22:30
@@ -60,12 +60,18 @@ env:
jobs:
compile:
name: Compile Buildpacks
runs-on: ubuntu-24.04
runs-on: ${{ inputs.ip_allowlisted_runner }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant about this change. The compile job only requires an allowlisted runner for the front-end buildpacks because the repository is private. Public repos don't need this and benefit from higher availability and docker rate limiting benefits using public runners. So switching to always use ip_allowlisted_runner would have a negative impact overall as all our buildpack repositories should (eventually) be public.

Contrast this with the jobs that absolutely require an allowlisted runner because they perform write actions against the repositories (e.g.; publish-github, update-builder). These jobs have no choice but to use the allowlisted runners.

To minimize impact on the more common case @mars, I think we should instead use one of the following alternatives:

  • introduce an optional public input (defaults to true) and apply a conditional to the runs-on that chooses between the public runner and ip_allowlisted_runner
  • run a step that calls the github api to determine if the repository is public or private and then chooses between the public runner and ip_allowlisted_runner
  • introduce a default_runner input (defaults to ubuntu-24.04) so private repos can pass in an alternative

Copy link
Member

@edmorley edmorley Sep 6, 2024

Choose a reason for hiding this comment

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

Yeah I'd prefer to use the public runners (at least longer term), given publishing from a private repo is not a use-case we should be supporting longer term.

I'm also a bit hesitant to publish those CNBs from a private repo at all, even temporarily. We've committed to our buildpacks being open source - and whilst I'm sure the plan is to open source the repo soon, I think it might be better for optics if that happens first before the CNBs are published and added to the builder?

Otherwise it comes across to the community as "here's something we worked on in private, surprise!". Plus the repo URL will appear in a bunch of places (for example, the https://registry.buildpacks.io listing or the output of pack builder inspect heroku/builder:24), and the links will 404 until the repo is public, which will come across as a bit exclusionary.

The repo open sourcing process has also gotten a lot faster recently (Rune said the .NET repo was made public the day after the request was submitted), so waiting until the repo is public shouldn't block things too much?

Copy link
Member Author

@mars mars Sep 6, 2024

Choose a reason for hiding this comment

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

Thanks @colincasey for offering alternatives that could get this working.

@edmorley We're not ready to open-source the project I'm working on… I was under the impression that as long as the GitHub & Docker repos are private, that we would not be leaking preliminary releases to the public.

Copy link
Member

@edmorley edmorley Sep 6, 2024

Choose a reason for hiding this comment

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

Ah I didn't realise they weren't ready.

The release automation here presumes things are public, since it also registers the CNB releases on https://registry.buildpacks.io/ , which (a) means a public listing, (b) I think would fail, since the upstream CNB registry has to pull the image to fetch metadata to populate the listing etc - which it couldn't do from a private Docker repo/registry.

outputs:
buildpacks: ${{ steps.generate-buildpack-matrix.outputs.buildpacks }}
version: ${{ steps.generate-buildpack-matrix.outputs.version }}
changelog: ${{ steps.generate-changelog.outputs.changelog }}
steps:
- name: Get token for GH application (Linguist)
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need to fetch the token if we're doing some type of writable action against the repo, right? I don't think we need this for the checkout step. It should work for private repos as long as it's using an allowlisted runner.

Copy link
Member Author

Choose a reason for hiding this comment

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

If checkout for read-only works without the GH app token, then I agree this is unnecessary.

@@ -278,6 +284,13 @@ jobs:
needs: [compile]
runs-on: ${{ inputs.ip_allowlisted_runner }}
steps:
- name: Get token for GH application (Linguist)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for moving this step? If not, maybe leave it where it was since it just seems like noise for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the step to before checkout, like the other jobs, but it sounds like that is unnecessary.

@mars
Copy link
Member Author

mars commented Sep 6, 2024

Using an internal preview strategy to avoid these conundrums, until we're ready for public launch.

@mars mars closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants