-
-
Notifications
You must be signed in to change notification settings - Fork 384
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 the ability to override app configurations at the command line #1542
Changes from 5 commits
5e62d3c
806886e
115bae2
ee2e2c3
8816ee0
d37db2b
5dfd972
ead16a7
7d102de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
The ``-C``/``--config`` option can now be used to override app settings from the command line. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ | |||||||||||||||||
from abc import ABC, abstractmethod | ||||||||||||||||||
from argparse import RawDescriptionHelpFormatter | ||||||||||||||||||
from pathlib import Path | ||||||||||||||||||
from typing import Any | ||||||||||||||||||
|
||||||||||||||||||
from cookiecutter import exceptions as cookiecutter_exceptions | ||||||||||||||||||
from cookiecutter.repository import is_repo_url | ||||||||||||||||||
|
@@ -103,6 +104,26 @@ def split_passthrough(args): | |||||||||||||||||
return args[:pos], args[pos + 1 :] | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
def parse_config_overrides(config_overrides: list[str] | None) -> dict[str, Any]: | ||||||||||||||||||
"""Parse command line -C/--config option overrides. | ||||||||||||||||||
|
||||||||||||||||||
:param config_overrides: The values passed in as configuration overrides. Each value | ||||||||||||||||||
*should* be a "key=<valid TOML>" string. | ||||||||||||||||||
:returns: A dictionary of app configuration keys to override and their new values. | ||||||||||||||||||
:raises BriefcaseCommandError: if any of the values can't be parsed as valid TOML. | ||||||||||||||||||
""" | ||||||||||||||||||
overrides = {} | ||||||||||||||||||
if config_overrides: | ||||||||||||||||||
for value in config_overrides: | ||||||||||||||||||
try: | ||||||||||||||||||
overrides.update(tomllib.loads(value)) | ||||||||||||||||||
except ValueError: | ||||||||||||||||||
raise BriefcaseCommandError( | ||||||||||||||||||
f"Unable to parse configuration override {value}" | ||||||||||||||||||
) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Rich's stacktrace handles this regardless but can you raise from the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should be unnecessary: exceptions raised within an except block are automatically chained unless you say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well be explicit. |
||||||||||||||||||
return overrides | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
class BaseCommand(ABC): | ||||||||||||||||||
cmd_line = "briefcase {command} {platform} {output_format}" | ||||||||||||||||||
supported_host_os = {"Darwin", "Linux", "Windows"} | ||||||||||||||||||
|
@@ -530,7 +551,11 @@ def finalize_app_config(self, app: AppConfig): | |||||||||||||||||
:param app: The app configuration to finalize. | ||||||||||||||||||
""" | ||||||||||||||||||
|
||||||||||||||||||
def finalize(self, app: AppConfig | None = None): | ||||||||||||||||||
def finalize( | ||||||||||||||||||
self, | ||||||||||||||||||
app: AppConfig | None = None, | ||||||||||||||||||
config_overrides: list[str] | None = None, | ||||||||||||||||||
): | ||||||||||||||||||
"""Finalize Briefcase configuration. | ||||||||||||||||||
|
||||||||||||||||||
This will: | ||||||||||||||||||
|
@@ -544,17 +569,25 @@ def finalize(self, app: AppConfig | None = None): | |||||||||||||||||
:param app: If provided, the specific app configuration | ||||||||||||||||||
to finalize. By default, all apps will be finalized. | ||||||||||||||||||
""" | ||||||||||||||||||
# Convert any configuration overrides provided at the command line | ||||||||||||||||||
# into values that can be applied to the app's config | ||||||||||||||||||
overrides = parse_config_overrides(config_overrides) | ||||||||||||||||||
|
||||||||||||||||||
self.verify_host() | ||||||||||||||||||
self.verify_tools() | ||||||||||||||||||
|
||||||||||||||||||
if app is None: | ||||||||||||||||||
for app in self.apps.values(): | ||||||||||||||||||
if hasattr(app, "__draft__"): | ||||||||||||||||||
self.finalize_app_config(app) | ||||||||||||||||||
for key, value in overrides.items(): | ||||||||||||||||||
setattr(app, key, value) | ||||||||||||||||||
delattr(app, "__draft__") | ||||||||||||||||||
else: | ||||||||||||||||||
if hasattr(app, "__draft__"): | ||||||||||||||||||
self.finalize_app_config(app) | ||||||||||||||||||
for key, value in overrides.items(): | ||||||||||||||||||
setattr(app, key, value) | ||||||||||||||||||
delattr(app, "__draft__") | ||||||||||||||||||
|
||||||||||||||||||
def verify_app(self, app: AppConfig): | ||||||||||||||||||
|
@@ -687,6 +720,14 @@ def add_default_options(self, parser): | |||||||||||||||||
|
||||||||||||||||||
:param parser: a stub argparse parser for the command. | ||||||||||||||||||
""" | ||||||||||||||||||
parser.add_argument( | ||||||||||||||||||
"-C", | ||||||||||||||||||
"--config", | ||||||||||||||||||
dest="config_overrides", | ||||||||||||||||||
action="append", | ||||||||||||||||||
metavar="KEY=VALUE", | ||||||||||||||||||
help="Override the value of the app configuration item KEY with VALUE.", | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: no period at the end to match the other options (although, I also shouldn't have added one to
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting - the very next option defined ( |
||||||||||||||||||
) | ||||||||||||||||||
parser.add_argument( | ||||||||||||||||||
"-v", | ||||||||||||||||||
"--verbosity", | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,7 @@ def __call__( | |
no_update: bool = False, | ||
test_mode: bool = False, | ||
passthrough: list[str] | None = None, | ||
config_overrides: list[str] | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs to be passed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but not any more with the revised approach that I've implemented. |
||
**options, | ||
) -> dict | None: | ||
# Which app should we run? If there's only one defined | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ def __call__( | |
update_resources: bool = False, | ||
update_support: bool = False, | ||
test_mode: bool = False, | ||
config_overrides: list[str] | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs to be passed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, Yes, but not any more with the revised approach that I've implemented. |
||
**options, | ||
) -> dict | None: | ||
# Confirm host compatibility, that all required tools are available, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any mention of multi-level config values, like
--config 'linux.requires=...'
, which is valid TOML.I think this code might actually work with that as long as you only had one of them. But then if you added
--config linux.something_else=...
, the secondlinux
would overwrite the first instead of merging with it.We don't necessarily need to support that syntax in this PR, but if we don't support it, we should at least display a useful error when the user tries to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Right now, it's probably easier to reject multi-level keys completely. We can always add them later if there's a reasonable use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually - in the process of looking at this, it occurred to me that this approach avoids validation of any provided value (e.g., you could specify an invalid version number)... and when I went looking to see how I could do validation, I found a much cleaner way to handle the overrides, in a way that both validates and avoids the issue @rmartin16 found with not passing the overrides to some commands.