-
Notifications
You must be signed in to change notification settings - Fork 42
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
Remove runtime config logic #597
Conversation
This pull request does not have a backport label. Could you fix it @jeniawhite? 🙏
|
Cloudbeat CI 🤖Allure Report: http://csp-allure-reports.s3.amazonaws.com/allure_reports/cloudbeat/prs/597/index.html |
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.
looking at the new config.v1 format, it seems like we moved the credentials to there also, yet we still read our credentials from the flat version. Not sure if we should also address this change here, to fully migrate to the new format wdyt? nvm out of scope
Also, did you test this E2E? @jeniawhite
// TODO: Should we check something? | ||
// Benchmark is being checked inside config.New | ||
_, err := config.New(cfg) |
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 feel like the validation inside config should be here
if c.Benchmark != "" {
if !isSupportedBenchmark(c.Benchmark) {
return c, ErrBenchmarkNotSupported
}
}
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've considered moving the validation to the validator but that would require us to move logic from the config
package to the beater
package.
Which requires recoding parts of the cloudbeat
like newCloudbeat
signatures and etc...
Reconsidering the existing solution I did not see any value in the validator
, because we use the same validation for non-fleet managed configs and reconfigurations.
This is why I've consolidated the validity checks in the config constructor and removed the validator.
Would be great to get your input on that @amirbenun.
@oren-zohar |
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.
IMO, we should keep decoupling cloudbeat and the launcher.
For that PR, it means that the validator should remain as an abstraction layer for beater specific use cases. Even if the validator only calls another validation function for the config.
This pull request is now in conflicts. Could you fix it? 🙏
|
@amirbenun Rolled back to the commit that leaves the |
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.
LGTM
Result: fetcherResult, | ||
ActivatedRules: o.activatedRules, | ||
Result: fetcherResult, | ||
Benchmark: o.benchmark, |
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 am missing some context here, did we change all the rules in a way they will run only when their benchmark is on?
@@ -17,7 +17,7 @@ | |||
|
|||
package version | |||
|
|||
const policyVersion = "v1.2.3" | |||
const policyVersion = "v1.2.4" |
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 guess that's your matching change for the rules?
What does this PR do?
Removes the runtime config logic
Checklist