-
Notifications
You must be signed in to change notification settings - Fork 115
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 generate command #361
Add generate command #361
Conversation
b0f76e5
to
dc21dc3
Compare
Alrighty, refactored a bit to avoid duplicated code. Should be ready for review 🙂 |
dc21dc3
to
1cb8397
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Some ideas from @Gaikanomer9 would be great to get into this PR then it should be good soon. 👍
filesystem.go
Outdated
|
||
// objectiveAsRuleFile reads a ServiceLevelObjective YAML manifest and outputs the corresponding | ||
// Prometheus rules as a file in the desired directory. | ||
func objectiveAsRuleFile(file, prometheusFolder string) (slo.Objective, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably cleaner if we can somehow figure out a way to only do the writes in this function and return the error at the end. The slo.Objective
could be created outside this function?
Probably we'd have a read and write (many) function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried splitting it into two separate functions, but not we're unmarshaling SLO files twice 😅
Is that what you had in mind?
61953bf
to
dffcc15
Compare
It would appear to me that the change in its current form does not parse multiple objects per yaml (like the Also, any chance for some forward movement on this PR? |
ah, interesting, I didn't notice this multiple objects per yaml problem. I'll try to take a look at it this week! Tips on how to solve are appreciated 😝 |
That was definitely a bug in the |
dffcc15
to
bb0c4e6
Compare
Nice! Thanks for solving this one 🥳 I'm stuck at another problem now, but seems like this problem also exists on I'm trying to run those commands, and both fail with the same error:
But if I use a direct path to a single file, it works as expected (only permission issues with the output folder):
On main the same behavior is happening for the |
This should work since the code uses |
Signed-off-by: ArthurSens <[email protected]>
bb0c4e6
to
1d85de1
Compare
Sorry for taking so long, but PR rebased! I believe it should be ready for a review 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you so much! 🎉
Let's get this in and send small patches if we encounter anything else.
Fixes #314
Just took a quick try into 314... Fixes the issue but I basically copied code from
filesystem.go
, so I believe the code can still be improved to reduce duplicated code😅Opening the PR early just in case you were imagining something different here 🙂