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

Fix stream issues in container.top #250

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

RazCrimson
Copy link
Contributor

@RazCrimson RazCrimson commented Mar 12, 2023

Changes:

The stream_helper would be useful when other APIs require streaming and JSON decoding like for the API mentioned in #239

Please do let me know in case something needs to be changed or amended.

Closes #249

@umohnani8
Copy link
Member

Changes LGTM
@rhatdan @lsm5 @jwhonce PTAL

yield json.loads(entry)
else:
yield entry
return json.loads(response.content) if decode else response.content
Copy link
Member

Choose a reason for hiding this comment

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

This changes the behaviour of stats() to read all the contents before returning the JSON payload. Given there is a streaming flag I don't see this change as wrong, just potentially breaking for developers who may be using the previous implementation.

Copy link
Contributor Author

@RazCrimson RazCrimson Mar 13, 2023

Choose a reason for hiding this comment

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

The change in loading the response was made in 3ad81a7
It has already been merged in #237 and released. 😅

Also the existing code before that had issues for non-stream mode, where it was encoding each character in the response with an \n appended. I ended up replacing it with a simple json.loads.

Snippet for Ref:

        with io.StringIO() as buffer:
            for entry in response.text:
                buffer.write(json.dumps(entry) + "\n")
            return buffer.getvalue()

Please feel free to suggest any better alternatives.
I'll make the required changes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce, RazCrimson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lsm5
Copy link
Member

lsm5 commented Mar 14, 2023

LGTM if @rhatdan and @jwhonce are happy.

@RazCrimson might wanna squash commits?

@RazCrimson RazCrimson force-pushed the fix-container-stream-issues branch from 1ab7515 to 7a9d1ed Compare March 14, 2023 12:41
@RazCrimson
Copy link
Contributor Author

@lsm5
Is this current level of squashing fine or would you like me to squash everything into one commit?

@lsm5
Copy link
Member

lsm5 commented Mar 14, 2023

@lsm5 Is this current level of squashing fine or would you like me to squash everything into one commit?

i was just checking if you needed / wanted to squash at all :) . Either way is fine by me.

Also, there's a ModuleNotFoundError now :(

@RazCrimson RazCrimson force-pushed the fix-container-stream-issues branch from 7a9d1ed to 3d8eb1b Compare March 14, 2023 13:29
@RazCrimson
Copy link
Contributor Author

Weird, there are no actual changes in the files before and after squashing.

Triggering another build did not fix it too.

Any ideas? 🤔

@lsm5
Copy link
Member

lsm5 commented Mar 15, 2023

@cevich any idea whatsup with the fedora-37 failure?

@cevich
Copy link
Member

cevich commented Mar 15, 2023

@cevich any idea whatsup with the fedora-37 failure?

Unfortunately no, I encountered the same/similar errors while trying to update the CI VM images over in #251

I think we need a python-guru to take a look.

@mwhahaha
Copy link
Contributor

Probably need to add extras into the requirements. It was removed from fixtures via testing-cabal/fixtures@f8df219 however we're using fixtures~=3.0.0. Could try bumping fixtures to 4.0.0 or including extras in the requirements.

@mwhahaha
Copy link
Contributor

Fixing CI via #252

@lsm5
Copy link
Member

lsm5 commented Mar 16, 2023

@mwhahaha thanks! @RazCrimson rebasing should make CI green.

include: tests for `stream_helper` and typing fixes in test_parse_utils.py

Signed-off-by: Raz Crimson <[email protected]>
fixes: Container.top
- infinite blocking when stream=True
- inconsistent decoding to json

add: test for Container.top with stream=True to avoid regression

Signed-off-by: Raz Crimson <[email protected]>
@RazCrimson RazCrimson force-pushed the fix-container-stream-issues branch from 3d8eb1b to 2438096 Compare March 16, 2023 12:35
@RazCrimson
Copy link
Contributor Author

@lsm5 Done!

@rhatdan
Copy link
Member

rhatdan commented Mar 16, 2023

LGTM

@lsm5 lsm5 merged commit a7342fb into containers:main Mar 16, 2023
@RazCrimson RazCrimson deleted the fix-container-stream-issues branch March 16, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming not supported in Container.top
7 participants