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

update /status response and docs #10189

Merged
merged 11 commits into from
Nov 12, 2021
Merged

update /status response and docs #10189

merged 11 commits into from
Nov 12, 2021

Conversation

carlad
Copy link
Contributor

@carlad carlad commented Nov 12, 2021

Closes #10130

Proposed changes:

  • The ensures the response payload has the correct key/values.
  • Updates the docs to conform to this.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog

@carlad carlad requested a review from a team as a code owner November 12, 2021 12:07
@carlad carlad requested review from aeshky, alwx and joejuzl and removed request for a team November 12, 2021 12:07
Copy link
Contributor

@alwx alwx 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, thanks for fixing this!

Copy link
Contributor

@joejuzl joejuzl 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!
I think we should add a changelog too though as it may affect users.

@carlad
Copy link
Contributor Author

carlad commented Nov 12, 2021

I've made a change to how the filename is being retrieved that differs from your suggestion @joejuzl .
The model_tar returns the full path to the file, not just the filename, so even though the tests were passing, locally the directory models was being returned in the payload.

Instead I added a model_filename attribute to the Processor, which uses the model_path to get_latest_model and then derives the os.path.basename from that.

The tests still pass, and I'm hoping that fixes things locally...

@carlad carlad dismissed joejuzl’s stale review November 12, 2021 17:08

The changelog has been updated.

@carlad carlad merged commit d08f8dc into main Nov 12, 2021
@carlad carlad deleted the 3.0-status-endpoint branch November 12, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API QA: /status endpoint returns a response body that's not consistent with the docs
4 participants