-
Notifications
You must be signed in to change notification settings - Fork 23
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks @iainlane for the contribution. We have moved the 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: |
There was a problem hiding this comment.
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:
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
Hi @iainlane, 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:
does not work. Is this correct? |
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:
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:
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.