Skip to content

docs: enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all #3665

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

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

NadavOps
Copy link
Contributor

enable_workflow_job_labels_check is no longer exist, correct?
and now enable_runner_workflow_job_labels_check_all is the functioning variable, correct?

This is just to swap in the READMEs and samples

@npalm npalm changed the title enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all dics: enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all Dec 16, 2023
@npalm npalm changed the title dics: enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all docs: enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all Dec 16, 2023
@npalm npalm changed the title docs: enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all docs: update non existing enable_workflow_job_labels_check Dec 18, 2023
@NadavOps NadavOps force-pushed the main branch 2 times, most recently from 124639b to d4180c0 Compare December 19, 2023 08:48
@NadavOps NadavOps changed the title docs: update non existing enable_workflow_job_labels_check enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all Dec 19, 2023
@NadavOps NadavOps changed the title enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all docs: enable_workflow_job_labels_check -> enable_runner_workflow_job_labels_check_all Dec 19, 2023
npalm
npalm previously approved these changes Dec 19, 2023
@npalm npalm enabled auto-merge (squash) December 19, 2023 09:30
@sdarwin
Copy link
Contributor

sdarwin commented Dec 20, 2023

With the PR, enable_runner_workflow_job_labels_check_all is being mentioned in the multi-runner README.md and variables.tf.

However, enable_runner_workflow_job_labels_check_all doesn't actually exist in the context of multi-runner. To check that:


cd terraform-aws-github-runner
grep -r "enable_runner_workflow_job_labels_check_all"

Its only appearance is in the root level variables.tf and main.tf. Do those apply in /examples/multi-runner/ or modules/multi-runner/? Correct me if this is wrong. I think what should happen, is to slightly refactor the code of multi-runner so that it uses the variable enable_runner_workflow_job_labels_check_all instead of exactmatch.

@npalm npalm disabled auto-merge December 20, 2023 21:21
@npalm
Copy link
Member

npalm commented Dec 20, 2023

@sdarwin I hope to have tomorrow some time to dig in the label part. It certainly require some owrk. And evenutally I think some reafactoring.

@NadavOps
Copy link
Contributor Author

NadavOps commented Jan 6, 2024

Did I fat finger close the PR? @npalm

@NadavOps
Copy link
Contributor Author

NadavOps commented Jan 6, 2024

BTW it seems it is mentioned in variables only in the description as it actually take it from another place @sdarwin

Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@npalm npalm merged commit d2558a1 into github-aws-runners:main Jan 12, 2024
@sdarwin
Copy link
Contributor

sdarwin commented Jan 12, 2024

it actually take it from another place @sdarwin

@npalm which place?

The variable "enable_runner_workflow_job_labels_check_all" is defined in the root level, top level, files variables.tf and main.tf.

https://github.com/philips-labs/terraform-aws-github-runner/blob/main/main.tf
https://github.com/philips-labs/terraform-aws-github-runner/blob/main/variables.tf

Does multi-runner reference those files?

@sdarwin
Copy link
Contributor

sdarwin commented Jan 12, 2024

There are two possibilities:

  1. A "minimal fix". The multi-runner example files should be configured to use "enable_runner_workflow_job_labels_check_all". Currently https://github.com/philips-labs/terraform-aws-github-runner/tree/main/examples/multi-runner doesn't mention that variable.

  2. A more in-depth fix as mentioned in my earlier comments.

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.

3 participants