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: improve the commands output #8

Merged
merged 5 commits into from
Sep 25, 2024
Merged

fix: improve the commands output #8

merged 5 commits into from
Sep 25, 2024

Conversation

MaferMazu
Copy link
Collaborator

@MaferMazu MaferMazu commented Sep 23, 2024

Description

This PR uses tutor_utils.execute() in the enable-private-packages and enable-themes commands to make debugging easier when they are inside the extra-commands command.

Pd: I couldn't use tutor_utils.execute() in extra-commands, because it uses chained commands.

More info about that util: https://github.com/overhangio/tutor/blob/v18.1.3/tutor/utils.py#L210

Also, check for themes to be dictionaries to avoid the type ignore comments.

How to test

  • If the tests pass, the commands work as expected (See the integration test for more info).
  • Run: tutor picasso run-extra-commands with this branch using the enable-themes and enable-private-packages inside. (See the screenshots for this test).
  • Search for the logs in a building process. Example: https://github.com/eduNEXT/ednx-strains/actions/runs/11021514034

Screenshots

Using in the config.yml:

PICASSO_EXTRA_COMMANDS:
- tutor plugins update
- tutor picasso enable-themes
- tutor picasso enable-private-packages
- tutor config save

Before
image

After
image

After merge

@MaferMazu MaferMazu linked an issue Sep 23, 2024 that may be closed by this pull request
@MaferMazu MaferMazu force-pushed the mfmz/improve-outputs branch 2 times, most recently from 6ad0fa1 to add7f2c Compare September 24, 2024 00:13
@MaferMazu MaferMazu requested review from mariajgrimaldi and a team September 24, 2024 04:18
tutorpicasso/commands/enable_themes.py Outdated Show resolved Hide resolved
tutorpicasso/commands/enable_themes.py Outdated Show resolved Hide resolved
@@ -80,6 +81,7 @@ def run_command(command: str) -> None:
Args:
command (str): Tutor command.
"""
click.echo(tutor_fmt.command(command))
Copy link
Contributor

@mariajgrimaldi mariajgrimaldi Sep 24, 2024

Choose a reason for hiding this comment

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

I don't want this to be a blocker since the design of running multiple commands is out of the scope of this PR, so please, don't consider it one.

Why can't we use tutor_utils.execute instead of directly calling subprocess.Popen? It will save us a lot of trouble since we won't have to handle errors or echo the command info manually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried, but it makes the chained elements weird.

For example:
Using: tutor_utils.execute(*command.split()) it returns:
tutor plugins update '|' tutor plugins index add https://raw.githubusercontent.com/eduNEXT/tutor-plugin-indexes/picasso_test/
To make it possible to allow chained elements, we would need to add shell=True in https://github.com/overhangio/tutor/blob/master/tutor/utils.py#L216

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found a way:
Using: tutor_utils.execute('sh', '-c', command)
The only thing that is not so good about this is that every command will have a sh -c prefix:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used this solution. Having the sh -c prefix is not good looking but improves the readability of the code and doesn't bother a lot in the logs.

If you have more feedback about this, please let me know.

Copy link
Contributor

@mariajgrimaldi mariajgrimaldi Sep 25, 2024

Choose a reason for hiding this comment

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

This looks significantly better and gives even more information for debugging purposes. I tested it and it's working as expected. Thank you!

Copy link

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

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

Hi @MaferMazu, thanks for the PR!
Could we show in the cover letter the before and after the commands output?

CHANGELOG.md Outdated Show resolved Hide resolved
@MaferMazu MaferMazu force-pushed the mfmz/improve-outputs branch 2 times, most recently from 9900b37 to 0d46c8d Compare September 24, 2024 23:42
@MaferMazu
Copy link
Collaborator Author

Thanks, @mariajgrimaldi @BryanttV, for the feedback. I applied it. I would love your re-review.

Copy link

@BryanttV BryanttV left a comment

Choose a reason for hiding this comment

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

LGTM!

@MaferMazu MaferMazu merged commit 793e152 into main Sep 25, 2024
7 checks passed
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.

Show commands output when running run-extra-commands
3 participants