-
Notifications
You must be signed in to change notification settings - Fork 164
add warning logs for experimental & beta components #507
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
base: main
Are you sure you want to change the base?
add warning logs for experimental & beta components #507
Conversation
c8cee1d to
8f369ec
Compare
8f369ec to
57d659d
Compare
|
Expecting the test github action to fail - but it would be fixed by: #508 |
Signed-off-by: Jem Davies <jemsot@gmail.com>
57d659d to
3b92545
Compare
|
@gregfurman - this is ready for review 🙏 |
| if code := common.RunService(c, opts, true); code != 0 { | ||
| os.Exit(code) | ||
| } |
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.
Why the change here?
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.
iirc - I added a new test - but that test would always fail because it would call os.Exit - now providing that the code returned isn't 0 we just let the go program terminate without calling os.Exit allowing the test to run
| err := service.RegisterInput( | ||
| "deprecated", | ||
| service.NewConfigSpec().Deprecated(), | ||
| func(conf *service.ParsedConfig, mgr *service.Resources) (service.Input, error) { | ||
| return deprecatedInputComponent{}, nil | ||
| }, | ||
| ) | ||
| require.NoError(t, err) |
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.
Can we rather do this against a local version of the environment? Unsure we should be testing against the global one.
| Usage: "Whether HTTP endpoints registered by stream configs should be prefixed with the stream ID", | ||
| }, | ||
| }, | ||
| Flags: streamModeFlags(), |
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.
How come we moved this out to its own 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.
Just thought it would be more tidy then the mixed approach we have with the flags at the moment.
| docs: $(APPS) $(TOOLS) | ||
| @$(PATHINSTTOOLS)/bento_docs_gen $(DOCS_FLAGS) | ||
| @$(PATHINSTBIN)/bento lint --deprecated "./config/examples/*.yaml" \ | ||
| @$(PATHINSTBIN)/bento lint --deprecated --allow-experimental --allow-beta "./config/examples/*.yaml" \ |
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.
What do you think about making these opt-in instead of opt-out? Like, --disable-experimental and --disable-beta to trigger these warnings.
Given how many users are likely using experimental or beta components, I'm concerned this is going to give the wrong impression of our plugin stability 😅
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 would have liked it such that we make it so you have to --allow-experimental to even run the stream with an experimental component. So instead of the warning we fatally terminate the program. That is rather heavy-handed though so went with the LintWarn approach. (Maybe we do that in a Major version update instead).
The way the component statuses work at the moment, i.e. experimental, beta & stable - other than the doc page warning text - we don't have much else in terms of protection. And to a user I suppose there is nothing else that differentiates them.
Because of that I feel we are reluctant to make breaking changes to experimental components. I feel that with these changes as-is - we might get more feedback from the community regarding if they are using an experimental component - and also we can be more assured to accept PRs with experimental components too.
I'm concerned this is going to give the wrong impression of our plugin stability
I agree - but I think that the fix to that is to do more 'promotion' of our existing components.
We can hold this PR off for now until we are happy with the current state of component statuses.
(Also would consider removing Beta entirely as a status).
Experimental - should be for new components - that might get changed further in the near future - and also might not have as many int. tests attached to them etc.
Stable - The majority of components should just be considered stable
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.
What do you think about making these opt-in instead of opt-out? Like, --disable-experimental and --disable-beta to trigger these warnings.
Feel like that approach might just bury the --disable-experimental CLI flag in the CLI help page and just won't get used.
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.
We could also just add a pre-run check that retrieves the ConfigView and checks if the component in question is stable or not. Would need to add a:
// IsStable returns true if the component is marked as stable.
func (c *ConfigView) IsStable() bool {
return c.component.Status == docs.StatusStable
}And then we can check:
isStable := GlobalEnvironment().GetProcessorConfig(name).IsStable() // or GetInputConfig etc.
if !isStable {
return fmt.Errorf("cannot create a processor of type %s: component is marked as unstable")
} Perhaps similar to what we do with our bundle approach and walk all processors before creating the manager:
var err error
GlobalEnvironment().WalkProcessors(func(name string, config *service.ConfigView) {
if !config.IsStable() {
err = fmt.Errorf("cannot create a processor of type %s: component is marked as unstable", name)
}
})
return errThere 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.
What are your thoughts on adding a grouped warning log instead of 1 per component? Perhaps that will make this change less scary to use.
For example:
[Warning] the following components are unstable and subject to changes outside of major versions: [ ... ]
Thoughts?
Co-authored-by: Greg Furman <31275503+gregfurman@users.noreply.github.com>
Bento components have an associated 'Status', that can take the value of "stable", "beta", "experimental" & "deprecated". Currently the only way a user of Bento would understand they were using a "beta" / "experimental" component is if they have seen the warning on the component's doc page.
This PR will add an additional type of Lint Warning such that when a "beta" or "experimental" component is parsed, we will emit a Warning log, users will be able to silence these warnings with additional options
--allow-beta&--allow-experimental.In a future major update to Bento, we could alter this behaviour so that Bento will not execute a stream with "experimental" or "beta" components, unless explicitly allowed by the user.
This PR will add warning logs when a stream is created via:
bento -c config.yamlbento streams ./*.yamlIn a follow up PR I will alter the StreamBuilder API to allow modifying if we will log warnings for experimental / beta components.