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

docs: exactMatch variable #3669

Closed
wants to merge 1 commit into from

Conversation

sdarwin
Copy link
Contributor

@sdarwin sdarwin commented Dec 14, 2023

Usage of the setting exactMatch is unclear.

There is an issue entitled "What's the point of exactMatch in multi-runner?" And I keep asking myself this question.

As an attempt to answer, this PR includes documentation which covers multiple cases of both exactMatch: true and exactMatch: false.

  • please verify the analysis is correct for each case
  • if possible, provide a situation which demonstrates the intended positive use of exactMatch: false. that can be added to the text.
  • if the conclusion is ultimately that exactMatch should be enabled, and is recommended, then we might wonder: why is the default setting false

@sdarwin sdarwin changed the title Docs: exactMatch variable docs: exactMatch variable Dec 15, 2023
@sdarwin
Copy link
Contributor Author

sdarwin commented Dec 15, 2023

Maybe there is another solution.

In the root level module it has a variable:

variable "enable_runner_workflow_job_labels_check_all" {
  description = "If set to true all labels in the workflow job must match the GitHub labels (os, architecture and `self-hosted`). When false if __any__ label matches it will trigger the webhook."
  type        = bool
  default     = true
}

That's used to populate exactMatch.

What could be done in multi-runner is:

  • Change all instances of exactMatch to be named enable_runner_workflow_job_labels_check_all. In the module, the examples, javascript source code, everywhere. No exactMatch.
  • Set the default to "true".
  • Then, remove enable_runner_workflow_job_labels_check_all from some of the multi-runner examples. When multi-runner has dozens of possible variables, they don't all need to be set in an example.
  • Document the purpose of enable_runner_workflow_job_labels_check_all with a concrete example. "Imagine this scenario, with these labels, and this workflow. You can see how it would be helpful to disable enable_runner_workflow_job_labels_check_all in this particular circumstance."

@npalm
Copy link
Member

npalm commented Dec 17, 2023

@sdarwin thanks for taking the time to improve the docs. Will check your PR in more detail asap. The labels are always confusing. In the meantime I would really appreciate if you have time to have a look at this PR (https://github.com/philips-labs/terraform-aws-github-runner/pull/3633). This PR moves all docs to a docs site. Feel free to make comments.

@sdarwin
Copy link
Contributor Author

sdarwin commented Dec 20, 2023

This PR should be reconsidered... See my previous comment, about a suggested strategy.

@sdarwin sdarwin closed this Dec 20, 2023
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.

2 participants