feat: Adding the possibility of removing the default labels

[jgutierrezglez]

In case the pool of runners deployed using this module are fully available for an wide GH org (no repository restrictions) any workflow configured to run in runners that contain just default labels in the runs-on definition:

e.g.

runs-on: self-hosted
runs-on: Linux

can end-up running in this pool without knowing it. That's why I have decided to remove the default labels from our runners and just rely on unique custom labels, and I believed the best way to do it by adding a runner_enable_default_labels variable - that by default is true (so, it doesn't change the current behavior), but it can help other people to deal with similar issues like the one described above.

[npalm]

I think I missed GitHub changes this. In the past the default labels are added by GitHub without control. When we introduced the JIT config the default labels are not added by default. But is this the same for non JIT?

[jgutierrezglez]

I think I missed GitHub changes this. In the past the default labels are added by GitHub without control. When we introduced the JIT config the default labels are not added by default. But is this the same for non JIT?

We're using JIT - and the default labels are added by default, that's why we have created this new variable and set it to false in the cases where we just want to use custom labels

[npalm]

Need to find some time to test.

• Changes impacted the runner module should also reflected to the multi-runner module. See comment
• I think this option requires that JIT config is enabled. Can you hadd this to the docs of the introduced variable.

[sdarwin]

For the reasons explained in the original post, this feature would be great, so you can enable runners without repository restrictions.

runs-on: self-hosted won't accidentally trigger a launch if only custom labels have been added.

Maybe I am mistaken (and I might be!) but here are a few quick comments:

1. This appears to be the pull request adding the feature: Add --no-default-labels config option to self-hosted runners actions/runner#2443

2. It doesn't mention JIT config. Isn't JIT completely orthogonal to default labels? Unrelated. So remove JIT from this PR.

3. The feature uses the flag --no-default-labels. Where is the flag --no-default-labels in this PR?

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions.

[npalm]

Sorry we had no time yet to dig in this PR. I do my best to check the PR in the coming week. Sorry for the delay.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions.

[jgutierrezglez]

@sdarwin I would have been great to be able to introduce a proper variable validation with TF 1.9, but in order to keep backwards compatibility, I updated the code to ignore runner_enable_default_labels = false (use the default value => true) in case enable_jit_config and enable_ephemeral_runners aren't set to true.

[sdarwin]

I don't understand how there is any connection between a new method of terraform variable validation, and what ought to be done here:

conditionally adding the "--no-default-labels" flag to config.sh.

[jgutierrezglez]

Reverted the last change and added the --no-default-labels to to config.sh when runner_enable_default_labels is set to true

[sdarwin]

@jgutierrezglez , great. I believe that is logically the correct step.
The next part, which maybe won't get added, is tests. CI tests. Even end-to-end tests in github actions. Is there any way to do that? Create a matrix of a small number of example values: with and without default labels. with and without enable_jit_config. Observe the outcomes, if the expected labels appear on the runner. If this were Ansible instead of Terraform, it could be done. https://ansible.readthedocs.io/projects/molecule/. Well, at least something to think about in the future. Maybe not now.

[sdarwin]

In the absence of automated CI tests, how about manual testing.

Run through these cases and report back the findings. Not what might be expected, but what the tests show. Post screen shots perhaps.

Let's say your chosen labels are [a,b,c]. We'll call them my_custom_labels. You can use any that you prefer.

Check in the Github control panel UI at https://github.com/_my_org_/_my_repo_/settings/actions/runners to see what labels are ultimately applied by Terraform. Let's call that "real resulting labels".

Fill in the blanks.

runner_enable_default_labels	your labels	enable_jit_config	enable_ephemeral_runners	real resulting labels
true	my_custom_labels	false	false
true	my_custom_labels	false	true
true	my_custom_labels	true	false
true	my_custom_labels	true	true
false	my_custom_labels	false	false
false	my_custom_labels	false	true
false	my_custom_labels	true	false
false	my_custom_labels	true	true

If all eight tests are correct, hopefully the maintainers will agree the PR is ready.

[sdarwin]

Windows modules\runners\templates\start-runner.ps1 with config.cmd is missing.

(add at least some windows tests)

[sdarwin]

How about this instead?

if [[ "$default_labels" == "true" ]]; then
    extra_flags="--no-default-labels"
else
    extra_flags=""
fi

sudo --preserve-env=RUNNER_ALLOW_RUNASROOT -u "$run_as" -- ./config.sh ${extra_flags} --unattended --name "$runner_name_prefix$instance_id" --work "_work" $${config}

[npalm]

This PR is still on my radar, but in the meantime started to refactor the webhook as well. See #4160 This change should not impact this PR, but good to be aware.

[npalm]

Thx the PR already looks quite good. But made some comments to suggest different names. Please can you also update the following parts.

• Update the windows start script to read the parameter
• Update docs, add the paramater: https://philips-labs.github.io/terraform-aws-github-runner/configuration/#aws-ssm-parameters
• Update the multi-runner module to pass the parameter down.

[npalm]

@jgutierrezglez feel free to grant maintainers access to write to the PR branch. There is a checkbox you can tick to that. In that case I can push my updates used for local testing as well.

[jgutierrezglez]

@jgutierrezglez feel free to grant maintainers access to write to the PR branch. There is a checkbox you can tick to that. In that case I can push my updates used for local testing as well.

It's already enabled, you should be able to push your updates