-
Notifications
You must be signed in to change notification settings - Fork 141
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 required filters from event streams #112
Remove required filters from event streams #112
Conversation
Checked commit imtayadeway@8f05a28 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
Agree, getting a single day is a little more painful than one might think it should be, off the top i cant recall any, but are there other endpoints that support |
@AllenBW any collection that has a times or dates (and I think all of them have created/updated on timestamps) will support it |
@imtayadeway so theeeeen i doubly agree with your initial sentiment, in order to maintain continuity throughout the api, an essential behavior! long live the Equal Sign™ |
what about MetricRollupsService ? it does similar validation, should that go too ? /cc @jntullo |
@abellotti we can change the MetricRollupsService, and probably should if this is the direction we choose to go. However, since the SUI is currently using it as is, I am not sure now would be the right time to implement that change since that would require changes on their part (thoughts @AllenBW ?). Since those are specific parameters, removing validation would likely be confusing for the user. If the validation were to go, we should then remove the parameter usage completely and make it work entirely off of the filters. |
Side note, preference is for everything to be a parameter... ALSO all the work being done that uses both of these services is here, still in wip so changing it isn't that big of a deal at the moment... Is it possible to only enable timestamp filter to support |
This LGTM!! Thanks @imtayadeway. |
Opening for discussion. My thoughts were that since we have a default limit/page size in place, that should be sufficient for preventing the user from asking for anything crazy. IMO having required filters seems a little draconian in forcing the user to use this API a certain way. For instance, one of the requirements was to enforce a minimum timestamp filter to be present. If we do this, that prevents the user from writing a more elegant query in the case that they want event streams for a certain day. It would be more elegant to use the
=
filter, but they have to use two - specifying both a<
and>
value./cc @AllenBW @gtanzillo
@miq-bot assign @abellotti