Skip to content

Conversation

@relwell
Copy link
Contributor

@relwell relwell commented Oct 13, 2025

Previously, we could define a runner group's name, but not its ID. This creates some challenges around specifying multiple runner groups and being able to retrieve information about them through GitHub's API. This allows us to set the group's ID using the same RUNNERS environment variable that allows us to set its name.

This change updates the README with the necessary information as well.

It's worth noting that per our existing README file, all configuration parameters can be set via the documented environment variables at runtime, or with a .env document made available in the working directory at runtime.

Closes #32
Closes #33
Closes #34
Closes #35

@relwell relwell requested a review from a team as a code owner October 13, 2025 20:47
@relwell relwell changed the title Support definiting group ID in environment Support defining group ID in environment Oct 14, 2025
@cjflan
Copy link
Contributor

cjflan commented Oct 14, 2025

If we are going to keep the architecture of one runner type per controller, does it make more sense to make this a part of the runners variable or split it out into its own env variable?

Alternative here

@relwell
Copy link
Contributor Author

relwell commented Oct 14, 2025

If we are going to keep the architecture of one runner type per controller, does it make more sense to make this a part of the runners variable or split it out into its own env variable?

Alternative here

I thought of that originally, too, but we're already instructing users to populate the environment variable with JSON, and so it makes sense to stay idiomatic there. I think that may come from ARC.

@cjflan
Copy link
Contributor

cjflan commented Oct 14, 2025

Sounds good, defining the runner name and ID together makes sense as well. We should update the error message on a failed parse to include a reference to the ID then too.

return nil, fmt.Errorf(`unable to parse the %s environment variable as a JSON array of runners. Make sure the variable is correctly set with a valid JSON array, for example, '[{"name":"my-test-runner"}]'`, RunnersEnvName)

@relwell
Copy link
Contributor Author

relwell commented Oct 15, 2025

Sounds good, defining the runner name and ID together makes sense as well. We should update the error message on a failed parse to include a reference to the ID then too.

return nil, fmt.Errorf(`unable to parse the %s environment variable as a JSON array of runners. Make sure the variable is correctly set with a valid JSON array, for example, '[{"name":"my-test-runner"}]'`, RunnersEnvName)

Will do, but it's worth keeping in mind that the ID should remain optional.

See:
image

@cjflan
Copy link
Contributor

cjflan commented Oct 15, 2025

The ID should stay (and is) optional. I was thinking of making sure that it referenced the ID in case someone set the runner like [{"name": "baz", "id": "2"}] and the parsing error would come from the ID part of the unmarshal, but that may be more hand holding than necessary.

Copy link
Contributor

@cjflan cjflan left a comment

Choose a reason for hiding this comment

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

LGTM

@cjflan
Copy link
Contributor

cjflan commented Oct 15, 2025

We should also add the documentation from the .env file to the README in this section

https://github.com/macstadium/orka-github-actions-integration?tab=readme-ov-file#environment-variables

@relwell
Copy link
Contributor Author

relwell commented Oct 15, 2025

@cjflan not sure we want to add it to the .env file, because most customers will only run a single runner group, and therefore can omit the field. I would put the cognitive burden on the more complex use case, if that makes sense.

@relwell relwell merged commit b84a9de into main Oct 16, 2025
2 checks passed
@relwell relwell deleted the define-runnergroup-id branch October 16, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants