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

Clarify caveats with custom job hooks after 6cf76ae8b #3622

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

okurz
Copy link
Member

@okurz okurz commented Dec 7, 2020

As it turned out reading job hook settings from configuration does not
work for a separate GRU service. Likely because this service never reads
the config file /etc/openqa/openqa.ini . Hence it is questionable if we
want to support reading from the generic config file considering that
the GRU service is standalone and can be run in a separate environment,
e.g. a separate host or container.

This commit removes the ineffective code for reading the config settings
and also clarifies some further caveats in the documentation, e.g.
apparmor rules as well as where the overall status can be found.

Related progress issue: https://progress.opensuse.org/issues/80736

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #3622 (9c3da00) into master (f3b3496) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3622   +/-   ##
=======================================
  Coverage   95.97%   95.97%           
=======================================
  Files         367      367           
  Lines       31926    31926           
=======================================
  Hits        30641    30641           
  Misses       1285     1285           
Impacted Files Coverage Δ
lib/OpenQA/Setup.pm 97.56% <ø> (ø)
t/10-jobs.t 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b3496...9c3da00. Read the comment docs.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I'm sure you can read the config from Minion jobs. That's what we do in other Minion jobs as well. It looks like it just didn't work because you didn't extend OpenQA::Setup::read_config to actually read the section/keys. So at lest the commit message seems room.

@kraih
Copy link
Member

kraih commented Dec 7, 2020

Yes, you can absolutely read the config from Minion jobs.

@okurz
Copy link
Member Author

okurz commented Dec 7, 2020

yes, but consider the example of running the GRU service elsewhere, e.g. in a separate container. Could it be we never read the config in GRU so far and it's also something that we do not want?

@Martchus
Copy link
Contributor

Martchus commented Dec 7, 2020

We currently read the config in GRU and actually rely on that already. If one runs GRU in a different container the config should be present in that container as well. The same counts for any other spin-off services like the websocket server, live handler and so on. Read my last comment to understand why the config from your last PR had no effect. It had nothing to do with GRU.

@okurz
Copy link
Member Author

okurz commented Dec 7, 2020

Yes, yes, I got that :)

@okurz okurz force-pushed the fix/hooks branch 3 times, most recently from d26155d to f3c8bab Compare December 7, 2020 14:25
@okurz
Copy link
Member Author

okurz commented Dec 7, 2020

updated:

  • One commit to clarify "caveats" in documentation, one commit to fix reading config by defining the config section header only, no default values to still allow dynamic definitions of config settings based on job result definitions without needing to spell out all of them explicitly

@okurz okurz force-pushed the fix/hooks branch 2 times, most recently from c469d65 to 6147410 Compare December 7, 2020 15:04
@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2020

This pull request is now in conflicts. Could you fix it? 🙏

This commit clarifies some further caveats in the documentation, e.g.
apparmor rules as well as where the overall status can be found.

Related progress issue: https://progress.opensuse.org/issues/80736
As it turned out reading job hook settings from configuration does not
work unless at least the section is explicitly mentioned in
lib/OpenQA/Setup. I only mention the section header and not any default
hook values as that would prevent reading any other, dynamically defined
hook config key.

Also tested the reading of config of dynamically defined config
parameters manually with

```
sudo -u geekotest perl -d script/openqa gru -m production run --reset-locks
```

Related progress issue: https://progress.opensuse.org/issues/80736
@okurz
Copy link
Member Author

okurz commented Dec 7, 2020

I recorded the failure of t/full-stack.t in https://progress.opensuse.org/issues/80800 and rebased to fix the conflict

@okurz okurz merged commit 4cd1f43 into os-autoinst:master Dec 7, 2020
@okurz okurz deleted the fix/hooks branch December 7, 2020 18:21
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.

None yet

3 participants