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.

Sign up for GitHub

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

Jump to bottom

Integrate daterange into datasets API #3371

Merged
merged 4 commits into from
Apr 10, 2023
Merged

Integrate daterange into datasets API #3371

merged 4 commits into from
Apr 10, 2023

Conversation

dbutenhof
Copy link
Member

PBENCH-1125

Remove the GET /datasets/daterange API in favor of a new query parameter, as GET /datasets?daterange. This allows getting the actual date range for any filtered selection of datasets.

@dbutenhof
Integrate daterange into datasets API
520f9ec 
PBENCH-1125

Remove the `GET /datasets/daterange` API in favor of a new query parameter, as
`GET /datasets?daterange`. This allows getting the actual date range for any
filtered selection of datasets.
@dbutenhof dbutenhof added Server API Of and relating to application programming interfaces to services and functions labels Apr 6, 2023
@dbutenhof dbutenhof self-assigned this Apr 6, 2023
webbnh
webbnh requested changes Apr 6, 2023
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement!!

However, I think the docs (and possibly the code) need a tweak around the behavior when both keysummary and daterange are specified. And, I have some suggestions/nits.

lib/pbench/server/api/resources/datasets_list.py Outdated Show resolved Hide resolved
lib/pbench/server/api/resources/datasets_list.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/server/test_datasets_list.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/server/test_datasets_list.py Outdated Show resolved Hide resolved
@webbnh
Copy link
Member

webbnh commented Apr 6, 2023

And, we need to get @npalaska to fix those flakey unit tests! 😉

@webbnh
Copy link
Member

webbnh commented Apr 6, 2023

The indirect cause seems to be here:

  File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pbench/server/database/models/users.py", line 132, in query
    dbsq = Database.db_session.query(User)
AttributeError: 'NoneType' object has no attribute 'query'

Apparently, somehow, the db_session is None...anyone have thoughts how that might (sometimes) happen?

@webbnh webbnh mentioned this pull request Apr 6, 2023
Merged
@dbutenhof dbutenhof marked this pull request as draft April 6, 2023 18:53
webbnh
webbnh previously approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I have an awkward question (or two) which probably means that we're missing a unit test.

docs/API/V1/list.md Show resolved Hide resolved
lib/pbench/test/unit/server/test_datasets_list.py Show resolved Hide resolved
webbnh
webbnh previously approved these changes Apr 7, 2023
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but for a typo. Also, I have a couple of suggestions.

docs/API/V1/list.md Outdated Show resolved Hide resolved
lib/pbench/server/api/resources/datasets_list.py Show resolved Hide resolved
lib/pbench/server/api/resources/datasets_list.py Show resolved Hide resolved
dbutenhof
dbutenhof commented Apr 7, 2023
View reviewed changes
Copy link
Member Author

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix the doc typo ...

lib/pbench/server/api/resources/datasets_list.py Show resolved Hide resolved
lib/pbench/server/api/resources/datasets_list.py Show resolved Hide resolved
@dbutenhof dbutenhof marked this pull request as ready for review April 7, 2023 20:29
@dbutenhof dbutenhof requested a review from webbnh April 7, 2023 20:29
webbnh
webbnh approved these changes Apr 7, 2023
View reviewed changes
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dbutenhof dbutenhof merged commit af615a2 into distributed-system-analysis:main Apr 10, 2023
@dbutenhof dbutenhof deleted the datefilter branch April 10, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riya-17 riya-17 riya-17 approved these changes

@webbnh webbnh webbnh approved these changes

@ndokos ndokos Awaiting requested review from ndokos

@MVarshini MVarshini Awaiting requested review from MVarshini

@npalaska npalaska Awaiting requested review from npalaska

Assignees

@dbutenhof dbutenhof

Labels
API Of and relating to application programming interfaces to services and functions Server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbutenhof @webbnh @riya-17