Switch 'osc.conf.config' from dict to Options class with type checking #1387

dmach · 2023-08-22T13:36:45Z

Fixes: #1371
Fixes: #1073
Obsoletes: #1377

codecov · 2023-08-22T13:38:59Z

Codecov Report

Patch coverage: 72.65% and project coverage change: +1.03% 🎉

Comparison is base (e8fc97b) 29.31% compared to head (c5e4f04) 30.35%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1387      +/-   ##
==========================================
+ Coverage   29.31%   30.35%   +1.03%     
==========================================
  Files          47       49       +2     
  Lines       17291    17685     +394     
==========================================
+ Hits         5069     5368     +299     
- Misses      12222    12317      +95

Files Changed	Coverage Δ
osc/connection.py	`53.26% <0.00%> (-0.27%)`	⬇️
osc/credentials.py	`43.57% <0.00%> (+1.36%)`	⬆️
osc/commandline.py	`19.08% <8.33%> (+0.02%)`	⬆️
osc/conf.py	`52.59% <66.24%> (+3.63%)`	⬆️
osc/util/models.py	`91.30% <91.30%> (ø)`
osc/util/xdg.py	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

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

osc/conf.py

osc/util/models.py

osc/conf.py

-
-
-def apply_option_types(config, conffile=""):
+class Password(collections.UserString):


ziQ · 2023-09-11T09:10:42Z

osc/connection.py

+
+            if options.cafile or options.capath:
+                ssl_context.load_verify_locations(cafile=options.cafile, capath=options.capath)


I just tried this with capath=False, which results in

... File "/usr/lib/python3.11/site-packages/osc/connection.py", line 268, in http_request ssl_context.load_verify_locations(cafile=options["cacert"], capath=False) TypeError: capath should be a valid filesystem path

... and with capath="", I get SSL Error: [X509: INVALID_DIRECTORY] invalid directory (_ssl.c:4154)

Nevermind that. With capath=None (which you default to) it works fine.

AFAIK False is an invalid value. It should either hold a path or be None.

Correct. I hadn't seen how you read the config (and your default values) due to "large diff hidden".

Correct. I hadn't seen how you read the config (and your default values) due to "large diff hidden".

I've rendered docs for this pull request, that might be helpful too:
https://opensuse-commander.readthedocs.io/en/readthedocs-test/api/osc.conf.html
https://opensuse-commander.readthedocs.io/en/readthedocs-test/oscrc.html

…e data validation

birkenfeld · 2023-10-10T12:54:25Z

One question about this change... so far, I've been using oscrc to store config for my own OSC plugin. Is this still supported, and if yes, what is the way to do it? My current way seems too hacky to be proper:

class MyOptions(osc.conf.Options):
    new_field: str = Field(...)
osc.conf.Options = MyOptions

dmach · 2023-10-10T13:52:32Z

It looks like it worked, but to be honest, I don't know if it was ever supported.
In my opinion plugins should not use oscrc to store their data.
But if there's a regression and there's a reasonable (non-hackish) way of fixing it, I'll do it.

If I broke something or if you need a good support for plugin configs,
could you open a new issue so we don't discuss it in a closed pull request?

birkenfeld · 2023-10-12T04:51:43Z

Sure, thanks for the quick reply!

github-advanced-security bot found potential problems Aug 22, 2023

View reviewed changes

osc/conf.py Fixed Show fixed Hide fixed

osc/conf.py Fixed Show resolved Hide resolved