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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove required filters from event streams #112

Merged

Conversation

imtayadeway
Copy link
Contributor

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

@miq-bot
Copy link
Member

miq-bot commented Oct 5, 2017

Checked commit imtayadeway@8f05a28 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@AllenBW
Copy link
Member

AllenBW commented Oct 5, 2017

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 = date filtering?

@imtayadeway
Copy link
Contributor Author

@AllenBW any collection that has a times or dates (and I think all of them have created/updated on timestamps) will support it

@AllenBW
Copy link
Member

AllenBW commented Oct 6, 2017

@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

@abellotti
Copy link
Member

what about MetricRollupsService ? it does similar validation, should that go too ? /cc @jntullo

@jntullo
Copy link

jntullo commented Oct 6, 2017

@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.

@AllenBW
Copy link
Member

AllenBW commented Oct 6, 2017

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 =?

@abellotti
Copy link
Member

This LGTM!! Thanks @imtayadeway.

@abellotti abellotti merged commit 80c94f5 into ManageIQ:master Oct 6, 2017
@abellotti abellotti added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 6, 2017
@imtayadeway imtayadeway deleted the event-streams-required-filters branch January 12, 2018 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants