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

Allow large runners to be configured by an input #135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iainlane
Copy link

Some of the reusable workflow jobs in here refer to large runners which exist within the upbound GitHub organisation. These can't be invoked by forks:

Called workflows cannot be queued onto self-hosted runners across organisations/enterprises. Failed to queue this job. Labels: 'e2-standard-8 , linux'.

If we make this into an input then callers can customise it themselves and still continue to use the reusable workflows to publish their providers.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I've run it here. What works is using the built-in runners like ubuntu-latest. Specifying our own runner labels doesn't work still. But at least this lets us run the reusable workflows. This inspired me to start a discussion thread on the github community.

Some of the reusable workflow jobs in here refer to large runners which exist within the upbound GitHub organisation. These can't be invoked by forks:

> Called workflows cannot be queued onto self-hosted runners across organisations/enterprises. Failed to queue this job. Labels: 'e2-standard-8 , linux'.

If we make this into an input then callers can customise it themselves and still continue to use the reusable workflows to publish their providers.
@Upbound-CLA
Copy link

Upbound-CLA commented Sep 19, 2023

CLA assistant check
All committers have signed the CLA.

@ulucinar
Copy link
Contributor

Thanks @iainlane for the contribution. We have moved the upbound/provider-azuread under to the crossplane-contrib org and we can no longer run the CI jobs that depend on these larger runners. I was about to open a PR that will parameterize these runners and saw this PR.

Do you still have bandwidth to work on this PR? I will leave a review for you. If not possible, I can rebase this PR and merge it, and implement the suggested changes in a separate PR on top yours.

Thank you very much, and extremely sorry for the late reply.

@@ -3,6 +3,10 @@ name: Run Uptest
on:
workflow_call:
inputs:
large-runner:
Copy link
Contributor

Choose a reason for hiding this comment

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

How about naming this parameter as:

Suggested change
large-runner:
workflow-runner:

The motivation is the supplied runner selector could be smaller or larger compared to a reference (standard) runner. So, it's better if we don't imply a size in the parameter name in my opinion (although the current use case is to have larger runners).

@@ -3,6 +3,10 @@ name: Provider CI
on:
workflow_call:
inputs:
large-runner:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the comment above regarding the parameter naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we will currently need a new parameter for the lint job. It's using a different runner label than the others in the current main HEAD. My suggestion would be something like lint-workflow-runner. We can also consider differentiating the other job parameters but I would prefer we do so when we need it.

@@ -3,6 +3,10 @@ name: Provider Publish Service Artifacts
on:
workflow_call:
inputs:
large-runner:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the comment above regarding the parameter naming.

@ulucinar
Copy link
Contributor

ulucinar commented Feb 28, 2024

Hi @iainlane,
I've seen the discussion you've started here:
https://github.com/orgs/community/discussions/67583

So, my understanding is that even when we parameterize the runner labels, specifying non-standard label(s) will not work, e.g., when we will be reusing this workflow and pass a parameter like:

jobs:
  ci:
    uses: upbound/uptest/.github/workflows/provider-ci.yml@main
    with:
      larger-runner: "\"Ubuntu-Jumbo-Runner\""
      ...

does not work. Is this correct?

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.

None yet

3 participants