-
Notifications
You must be signed in to change notification settings - Fork 24
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
Merge confgen & main models (#254) #267
Conversation
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.
Thanks for refactoring and cleaning up the code :)
return map[string]interface{}{ | ||
"log-level": "error", | ||
"pipeline": pipeline.GetStages(), | ||
"parameters": pipeline.GetStageParams(), |
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.
Should we extract the field names to a struct? (something like the removed ConfigFileStruct
)
It could also replace the Output
type in the tests:
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.
Good point I'll take it back.
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.
(but now I want to also remove globals from config
😅
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.
done
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 think the replacement of the Output
type in the tests was missed.
@ronensc feedback addressed .. I actually ended up doing more changes in order to remove globals from |
Codecov Report
@@ Coverage Diff @@
## main #267 +/- ##
==========================================
+ Coverage 60.40% 66.72% +6.31%
==========================================
Files 72 73 +1
Lines 4213 4144 -69
==========================================
+ Hits 2545 2765 +220
+ Misses 1501 1199 -302
- Partials 167 180 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Fixes netobserv#254 - Removed duplicated confgen model - Use PipelineBuilder to simplify pipeline generation - Make confgen easier to consume as a lib (e.g. for NOO) - Do not make it necessary to call "Run": parsing definition files should be sufficient - Do not make it necessary to work with files written on disk: work with []byte instead - SkipWithTags should not result in an error when a file is skipped - Add confgen tests
A side-effect of removing globals in write_loki is that it changes how the config is read, and set with defaults. Instead of unmarshaling a second time to automatically get defaults, we now call an explicit function that sets the default. Also, now removing loki URL default, it now has to be set explicitely
Also fixed jsonnet dir actually used as filename prefix rather than directory
Co-authored-by: Ronen Schaffer <[email protected]>
Use main model in confgen, use pipeline builder
Note, changes required in NOO
Fixes #254
should be sufficient
with []byte instead