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

🐛 [BUG] - Loading SLOs from directory only loads .yaml files #311

Open
1 task done
mveroone opened this issue Dec 27, 2022 · 3 comments
Open
1 task done

🐛 [BUG] - Loading SLOs from directory only loads .yaml files #311

mveroone opened this issue Dec 27, 2022 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@mveroone
Copy link
Contributor

mveroone commented Dec 27, 2022

SLO Generator Version

2.3.3

Python Version

3.7.7

What happened?

When Loading SLOs from a directory (compute -f /etc/cronfig/slos -c config.yaml), this code is executed :
https://github.com/google/slo-generator/blob/master/slo_generator/utils.py#L64

And thus, only .yaml files are loaded, while the docs says any YAML or JSON file works.

I found out the hard way spending a lot of time trying to figure out why my .yml was only loaded if named explicitly.
Same as last time, I'll gladly work on a PR but we're still trying to figure out how to consistently sign CLAs company-wide. I hope to get this sorted by mid-January

What did you expect?

Any json/yml/yaml file would be loaded.
Ideally, all files would be loaded and tested for any of those 2 formats before being read or not.

Relevant log output

slo_generator.utils - DEBUG - Path '/etc/config/slos' not found. Trying to load from string
slo_generator.utils - ERROR - Error serializing config into dict. This might be due to a syntax error in the YAML / JSON config file.
slo_generator.utils - DEBUG - '/etc/config/slos'

Code of Conduct

  • I agree to follow this project's Code of Conduct
@mveroone mveroone added bug Something isn't working triage labels Dec 27, 2022
@lvaylet
Copy link
Collaborator

lvaylet commented Jan 2, 2023

Hi @mveroone and thanks for raising this issue.

I agree there is a discrepancy between what the doc says and what the code actually does. On top of that, get_files() looks for files whose extensions is one of yaml, yml or json. I understand it can be confusing.

I never noticed this error myself as I only use YAML files. I find them less verbose and more readable than JSON. This being said, there is no reason for not supporting JSON files if the doc says so.

Please let me know how the signing of the CLA goes. I can move forward on my own with the PR if fixing this behavior is both urgent and important.

@mveroone
Copy link
Contributor Author

mveroone commented Jan 2, 2023

Hi.

No urgency on my side, as the simple workaround was to have .yaml files so a simple renaming worked.
I'll try myself at the PR if you don't mind as soon as the CLA is signed.

Maxime

@lvaylet
Copy link
Collaborator

lvaylet commented Jan 2, 2023

Sure, that works for me!

@lvaylet lvaylet assigned mveroone and unassigned lvaylet Jan 6, 2023
@lvaylet lvaylet removed the triage label Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants