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

Use SSE to track CRC status change and logs #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evidolob
Copy link
Contributor

This PR depends on crc-org/crc#3903

PR adds usage new SSE channels for tracking CRC status change and receiving logs.

}, 3000);
const crcVersion = await getCrcVersion();
if (!crcVersion) {
console.warn("Can't get CRC CLI version, ignore getting daemons logs...");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.warn("Can't get CRC CLI version, ignore getting daemons logs...");
console.warn('Can't get CRC CLI version, ignore getting daemons logs...');

@gbraad gbraad requested a review from jeffmaury December 7, 2023 08:52
@gbraad
Copy link
Contributor

gbraad commented Dec 7, 2023

LOL, I just re-requested, while you had just reviewed. Sorry.

Just wanted to make sure you had seen this.
We are in the process of adding this to the CRC daemon.

src/constants.ts Outdated Show resolved Hide resolved
src/crc-delete.ts Outdated Show resolved Hide resolved
src/crc-status.ts Outdated Show resolved Hide resolved
src/crc-status.ts Outdated Show resolved Hide resolved
src/daemon-commander.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

I had CRC being stopped: when I launched Podman Desktop, I have this:

image

Is this expected that I can not start CRC from this screen ?

let discardTrailingNewline = false;

req.on('data', chunk => {
if (!buf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a lot of processing to handle the end (linefeed) of a message. Can this be extracted and tested separately? At the moment, this looks more like implied knowledge.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/preferences.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved

if (lineLength < 0) {
startingPos = length - pos;
startingFieldLength = fieldLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case it is a field, right?

can we therefore parse this as:

this.parseEventStreamField(buf, pos, fieldLength) ?

And the other case as:

this.parseEventStreamLine(buf, pos, lineLength); ?

If seems now to rely on a singe function with another check. This looks more complex than it needs to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is for case when we don't receive 'full' line, as it is not guaranty that we will have whole line
so we just make sure that we have a complete line before parse it.

@evidolob
Copy link
Contributor Author

evidolob commented Dec 7, 2023

@jeffmaury Have you use CRC from crc-org/crc#3903 PR?
As status change events is not merged in CRC yet, so you need to build CRC from that PR.

The reason why you may have error with latest CRC is because of this PR was check for wrong version - #192 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

3 participants