Skip to content

Conversation

@padovan
Copy link
Contributor

@padovan padovan commented Jan 16, 2025

We have proved that these cmds are useful. Now we need to do a serious cleanup as we move to increase our user base and expand what the kci-dev tooling can do.

@padovan
Copy link
Contributor Author

padovan commented Jan 16, 2025

cc @nuclearcat @aliceinwire
(For some reason, I can't Request a review in GitHub UI.

Copy link

@chantra chantra left a comment

Choose a reason for hiding this comment

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

I did not go through the whole file, but my meta comment is that whatever is currently printed by click should go to stderr, this is essentially a "logging" backend.
Logging should go to stderr in order to keep stdout pristine with content that is parseable, be it printing json content, or any potential format.

This will enable kci-dev as a pipeable command which can do the heavy lifting of pulling stuff from the API, and empowering people to extract content return by that api without having to create their own tool.

"Cloning repository (this might take significant time!)", fg="green"
)
repo = Repo.clone_from(giturl, workdir)
repo.git.checkout(branch)
else:
click.secho("Pulling repository", fg="green")
Copy link

Choose a reason for hiding this comment

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

same here, this would go to stdout potentially messing with parseable content from stdout.

@padovan
Copy link
Contributor Author

padovan commented Jan 17, 2025

I did not go through the whole file, but my meta comment is that whatever is currently printed by click should go to stderr, this is essentially a "logging" backend. Logging should go to stderr in order to keep stdout pristine with content that is parseable, be it printing json content, or any potential format.

This will enable kci-dev as a pipeable command which can do the heavy lifting of pulling stuff from the API, and empowering people to extract content return by that api without having to create their own tool.

Yeah, that makes sense. Let me update it to reflect this.

@padovan padovan force-pushed the maestro_common branch 2 times, most recently from 30d6cbb to b8c4a7d Compare January 17, 2025 11:46
@padovan padovan marked this pull request as ready for review January 17, 2025 15:08
@aliceinwire aliceinwire added this to the v0.1.2 milestone Jan 17, 2025
@@ -0,0 +1,19 @@
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

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

This file is not called from any other file

@padovan padovan force-pushed the maestro_common branch 2 times, most recently from 4ce99f6 to 910aa89 Compare January 17, 2025 18:44
@aliceinwire
Copy link
Member

aliceinwire commented Jan 17, 2025

@padovan please update the branch and rebase over main

Add colon punctuation to separate items better.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
The checkout command in printing the same information multiple
times before the nodes change again. Let's print it once.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
The bisect command doesn't call into the Maestro API, so
we don't need to manipulate such code here.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
We are already duplicating a lot of code, even in such an
earlier phase of the project. Let's change this trend and
start consolidating our code base.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Consolidate our API error message func. This will
facilitate future usage.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
By returning a json, we can immediatelly parse it in the
cmdline with 'jq'. That will help our scripting.

Ideally we would have a --json for this, but we can change this
later as kci-dev didn't reach 1.0 yet.

Suggested-by:Manu Bretelle <chantra@meta.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
@aliceinwire aliceinwire merged commit a3471ac into kernelci:main Jan 19, 2025
2 checks passed
Comment on lines +13 to +14
if data:
kci_log(json.dumps(data, indent=4))
Copy link

@chantra chantra Jan 21, 2025

Choose a reason for hiding this comment

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

So it looks like this is going to go to stderr, and not stdout because kci_log logs to stderr. But the log above is going to stdout.

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.

3 participants