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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generate command #361

Merged

Conversation

ArthurSens
Copy link
Contributor

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 馃檪

@ArthurSens ArthurSens marked this pull request as draft July 6, 2022 01:28
@ArthurSens ArthurSens force-pushed the arthursens/standalone-cli-tool-314 branch from b0f76e5 to dc21dc3 Compare July 6, 2022 14:51
@ArthurSens ArthurSens marked this pull request as ready for review July 6, 2022 14:52
@ArthurSens
Copy link
Contributor Author

Alrighty, refactored a bit to avoid duplicated code. Should be ready for review 馃檪

@ArthurSens ArthurSens force-pushed the arthursens/standalone-cli-tool-314 branch from dc21dc3 to 1cb8397 Compare July 6, 2022 14:56
Copy link
Member

@metalmatze metalmatze left a 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. 馃憤

generate.go Outdated Show resolved Hide resolved
filesystem.go Outdated Show resolved Hide resolved
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) {
Copy link
Member

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?

Copy link
Contributor Author

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?

@mmazur
Copy link
Contributor

mmazur commented Aug 1, 2022

It would appear to me that the change in its current form does not parse multiple objects per yaml (like the examples/nginx.yaml has). Is that a bug?

Also, any chance for some forward movement on this PR?

@ArthurSens
Copy link
Contributor Author

It would appear to me that the change in its current form does not parse multiple objects per yaml (like the examples/nginx.yaml has). Is that a bug?

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 馃槤

@metalmatze
Copy link
Member

That was definitely a bug in the examples/nginx.yaml 馃槄
It's fixed now, thanks to @mmazur 馃帀

@ArthurSens ArthurSens force-pushed the arthursens/standalone-cli-tool-314 branch from dffcc15 to bb0c4e6 Compare August 11, 2022 23:50
@ArthurSens
Copy link
Contributor Author

That was definitely a bug in the examples/nginx.yaml 馃槄 It's fixed now, thanks to @mmazur 馃帀

Nice! Thanks for solving this one 馃コ

I'm stuck at another problem now, but seems like this problem also exists on main.

I'm trying to run those commands, and both fail with the same error:

gitpod /workspace/pyrra (main) $ ./pyrra filesystem --config-files examples/*.yaml
pyrra: error: unexpected argument examples/prometheus-http.yaml
gitpod /workspace/pyrra (main) $ ./pyrra generate --config-files examples/*.yaml
pyrra: error: unexpected argument examples/prometheus-http.yaml

But if I use a direct path to a single file, it works as expected (only permission issues with the output folder):

gitpod /workspace/pyrra (main) $ ./pyrra filesystem --config-files examples/nginx.yaml
level=info ts=2022-08-12T00:17:01.716103492Z caller=main.go:119 msg="using Prometheus" url=http://localhost:9090
level=info ts=2022-08-12T00:17:01.716181202Z caller=filesystem.go:128 msg="watching directory for changes" directory=examples
level=info ts=2022-08-12T00:17:01.716410362Z caller=filesystem.go:245 msg="starting up HTTP API" address=:9444
level=debug ts=2022-08-12T00:17:01.716517122Z caller=filesystem.go:170 msg=reading file=examples/nginx.yaml
level=error ts=2022-08-12T00:17:01.720097441Z caller=filesystem.go:253 msg="failed to run" err="failed to create rule file \"examples/nginx.yaml\": failed to write file \"/etc/prometheus/pyrra/nginx.yaml\": open /etc/prometheus/pyrra/nginx.yaml: permission denied"
gitpod /workspace/pyrra (main) $ ./pyrra generate --config-files examples/nginx.yaml
level=info ts=2022-08-12T00:17:17.616300781Z caller=main.go:119 msg="using Prometheus" url=http://localhost:9090
level=error ts=2022-08-12T00:17:17.618056421Z caller=generate.go:36 msg="generating rule files" err="failed to write file \"/etc/prometheus/pyrra/nginx.yaml\": open /etc/prometheus/pyrra/nginx.yaml: permission denied"

On main the same behavior is happening for the filesystem command 馃. Is that expected?

@metalmatze
Copy link
Member

This should work since the code uses filepath.Glob to discover files in a directory.
I'll check out this PR locally to take a look.

Signed-off-by: ArthurSens <arthursens2005@gmail.com>
@ArthurSens ArthurSens force-pushed the arthursens/standalone-cli-tool-314 branch from bb0c4e6 to 1d85de1 Compare January 11, 2023 19:08
@ArthurSens
Copy link
Contributor Author

Sorry for taking so long, but PR rebased! I believe it should be ready for a review 馃檪

Copy link
Member

@metalmatze metalmatze left a 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.

@metalmatze metalmatze merged commit 335b597 into pyrra-dev:main Jan 12, 2023
@ArthurSens ArthurSens deleted the arthursens/standalone-cli-tool-314 branch January 12, 2023 11:18
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.

Standalone cli tool able to transform SLO definitions into prom rules
3 participants