quipucords / camayoc

Test automation framework that facilitates functional testing of quipucords.
https://camayoc.readthedocs.io/
GNU General Public License v3.0
5 stars 4 forks source link

DISCOVERY-296 - Clean up configuration file format #390

Closed mirekdlugosz closed 1 year ago

mirekdlugosz commented 1 year ago

Camayoc configuration file grew organically and I feel cleaning it up is necessary first step for further work.

Please see DISCOVERY-296 and commit messages for context and rationale behind these changes. Feel free to discuss both here.

This PR is best reviewed commit-by-commit. I would also ask to not squash commits during merge.

One visible problem of this PR is that if user has camayoc configuration file in $XDG_CONFIG_HOME, then it must be valid, even if they only want to use function to validate another file. The solution would be dynamic configuration loading on first request, similar to what Django does, but that also forces proxy object to maintain exact same interface as pydantic model, or code completion in editor won't work. I feel that would add unreasonable maintenance burden for tool as specialized as camayoc.

I didn't touch most of existing get_config calls. They should be all ported to new settings global object, but I think we can do it as separate task, or gradually as we work on the framework (basically - all new tests should be using settings, and port get_config calls when you work on functions that use it). But I can do it in another commit here, if people feel this is better approach.

Similarly, there is some duplication between pydantic models introduced here and attrs models used in UI tests. I can work on this as part of this PR, but I feel we can try to tackle this later, maybe as part of DISCOVERY-280.

codecov[bot] commented 1 year ago

Codecov Report

Merging #390 (5901b82) into master (b3980fb) will increase coverage by 0.66%. The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   79.91%   80.57%   +0.66%     
==========================================
  Files           5        5              
  Lines         229      242      +13     
==========================================
+ Hits          183      195      +12     
- Misses         46       47       +1     
Impacted Files Coverage Δ
camayoc/utils.py 78.33% <75.00%> (-2.62%) :arrow_down:
camayoc/config.py 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mirekdlugosz commented 1 year ago

I have run API and CLI tests against this branch. It also helped to pin-point few issues, that I addressed in new commits.

My local run has id in ibutsu 066baa00-58dd-4765-8004-2c752f37eaa5 (we should not link to internal systems in public projects). Compared to standard regression run, e.g. 032ce26c-be1d-4717-96b4-e827f5fc3011, I don't notice any new regressions.

032ce26c-be1d-4717-96b4-e827f5fc3011 (Jenkins run from master): Screenshot from 2023-03-31 20-16-39

066baa00-58dd-4765-8004-2c752f37eaa5 (this branch, ran locally): Screenshot from 2023-03-31 20-16-49

mirekdlugosz commented 1 year ago

I had to add .yamllint to make CI pass.

I had to ignore "truthy" rule for workflow files, because they reported:

3:1       warning  truthy value should be one of [false, true]  (truthy)

This is tool issue - it doesn't understand that on key is for GitHub to know when to run workflow. It sees "on" and thinks it's truthy value.

Also, I added scripts to ignored, because it's picking up issues in Ansible playbooks there. One of these playbooks will be removed after #389 , the second I'm not sure if even works.