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

Add SYNC_ASSETS_HOOK support #6131

Merged
merged 1 commit into from
Feb 6, 2025
Merged

Conversation

marmarek
Copy link
Contributor

This is useful when having site-local cache shared among several worker
hosts. See added documentation for scenario details.

Question: Should this setting be also exclusive to workers.ini, similar to GENERAL_HW_CMD_DIR and few others?

Copy link

Great PR! Please pay attention to the following items before merging:

Files matching docs/*.asciidoc:

  • Consider generating documentation locally to verify it is rendered correctly using tools/generate-docs

This is an automatically generated QA checklist based on modified files.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.99%. Comparing base (5f42d4f) to head (6960512).
Report is 57 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6131   +/-   ##
=======================================
  Coverage   98.99%   98.99%           
=======================================
  Files         396      396           
  Lines       39611    39618    +7     
=======================================
+ Hits        39211    39218    +7     
  Misses        400      400           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is useful when having site-local cache shared among several worker
hosts. See added documentation for scenario details.
@marmarek
Copy link
Contributor Author

marmarek commented Feb 5, 2025

ping?

@mergify mergify bot merged commit e206546 into os-autoinst:master Feb 6, 2025
46 checks passed
os-autoinst-bot pushed a commit to os-autoinst-bot/openQA that referenced this pull request Feb 7, 2025
commit e206546
Merge: 493cce4 6960512
Author:     mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
AuthorDate: Thu Feb 6 17:00:31 2025 +0000
Commit:     GitHub <[email protected]>
CommitDate: Thu Feb 6 17:00:31 2025 +0000

    Merge pull request os-autoinst#6131 from marmarek/sync-assets-hook

    Add SYNC_ASSETS_HOOK support
@perlpunk
Copy link
Contributor

perlpunk commented Feb 7, 2025

I would suggest to add that new variable here: https://github.com/os-autoinst/openQA/blob/master/lib/OpenQA/Worker/Job.pm#L240
We don't want people to add that setting to a job; it's supposed to be only set by the workers.ini, right?

@marmarek
Copy link
Contributor Author

marmarek commented Feb 7, 2025

Sure I can add it there. I included question about it in the PR description:

Question: Should this setting be also exclusive to workers.ini, similar to GENERAL_HW_CMD_DIR and few others?

TBH, I'm not sure what is the rule what settings are allowed only in workers.ini and what can be set via jobs. There are many variables that can take paths that may or may not be specific to individual worker filesystem layout (like several of GENERAL_HW_*_CMD, UEFI_PFLASH_*, EXTERNAL_VIDEO_ENCODER_CMD, just to name a few), yet only some of them are limited to workers.ini.

@perlpunk
Copy link
Contributor

perlpunk commented Feb 7, 2025

It's possible that there are more variables that should be only allowed via workers.ini.
I already talked about that in the past, and I think it's a bit unfortunate that all are ending up in the same variable.
Anything that can mess with the host system running qemu should probably forbidden.
Scheduling jobs cannot be done by any random person, so there is some protection, but another layer of protection can't hurt, especially since we can schedule jobs via PR comments now.

perlpunk added a commit to perlpunk/openQA that referenced this pull request Feb 7, 2025
SYNC_ASSETS_HOOK was added in os-autoinst#6131

Since it is a setting supposed for the workers.ini, we should disallow
it in the job settings.
@perlpunk
Copy link
Contributor

perlpunk commented Feb 7, 2025

I created #6160

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.

4 participants