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

Add launcher cli arg to check_config (New) #757

Merged
merged 4 commits into from
Oct 9, 2023
Merged

Conversation

Hook25
Copy link
Collaborator

@Hook25 Hook25 commented Oct 6, 2023

Description

We have a check-config command that is very useful to debug a config/check it before using it. This command does not support checking a launcher directly.

Resolved issues

N/A

Documentation

Added the documentation for this option in the correct place of the tutorial (where it is originally introduced) with an example run

Tests

Added a new test that covers the addition of the parameter, the other part was already well tested.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #757 (12c5f7a) into main (7395945) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #757      +/-   ##
==========================================
- Coverage   34.20%   34.19%   -0.01%     
==========================================
  Files         302      302              
  Lines       34134    34135       +1     
  Branches     5909     5909              
==========================================
- Hits        11674    11673       -1     
- Misses      21919    21920       +1     
- Partials      541      542       +1     
Flag Coverage Δ
checkbox-ng 60.99% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
checkbox-ng/checkbox_ng/launcher/check_config.py 93.10% <100.00%> (+0.24%) ⬆️

... and 1 file with indirect coverage changes

@kissiel
Copy link
Contributor

kissiel commented Oct 6, 2023

We have a check-config command that is very useful to debug a config/check it before using it. This command does not support checking a launcher directly.

Cause it's not meant to do that. The original intent was that Checkbox would start up as it normally would and print all the configs instead (and where they come from).

This feels like it should be an option to the launcher subcmd (implied or not).

The way it's here would be: checkbox-cli check-config ./some.launcher right?

@Hook25
Copy link
Collaborator Author

Hook25 commented Oct 6, 2023

It is pretty nice as checkbox-cli check-config ./some.launcher because it shows everything, origin of config values, problems in the launcher or other configs etc. I would not introduce a new command, the launcher is a special config, you use it by putting it at the end of a cli invocation, it should work with check-config I think

Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Good stuff, let's land this, but if we decide to make this also work for remote we need to rework it a bit, so we don't replicate the remote pipeline in order to get the values.

@Hook25 Hook25 merged commit 56b3088 into main Oct 9, 2023
19 of 20 checks passed
@Hook25 Hook25 deleted the add_launcher_check_config branch October 9, 2023 07:33
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.

2 participants