-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat(#702): serviceaccount removal #724
base: main
Are you sure you want to change the base?
Changes from all commits
1e1f430
6d00f86
ec42740
c66cfcf
cfafaa1
c561c4b
fdcbbfc
55ddecc
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 |
---|---|---|
|
@@ -48,8 +48,9 @@ def install(ctx, component, preset, apply, wait, **kwargs): | |
from alive_progress import alive_bar | ||
from gefyra import api | ||
|
||
if not all(kwargs.values()): | ||
kwargs = {} | ||
# filter out empty kwargs | ||
kwargs = {k: v for k, v in kwargs.items() if v} | ||
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. It would be more transparent to show a warning when things are filtered out no? Otherwise users might wonder. |
||
|
||
if wait and not apply: | ||
raise click.BadOptionUsage( | ||
option_name="wait", message="Cannot wait without '--apply'" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,14 @@ def data(params: "GefyraInstallOptions") -> list[dict]: | |
) | ||
if params.service_annotations: | ||
try: | ||
stowaway_annotations.update(params.service_annotations) | ||
for annotation in params.service_annotations: | ||
try: | ||
# handle cli params as key=value | ||
key, value = annotation[0].split("=") | ||
stowaway_annotations[key] = value | ||
except ValueError: | ||
# handle preset values | ||
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. We should probably display a warning or something if this happens and the parameter is ignored no? |
||
stowaway_annotations.update(params.service_annotations) | ||
except IndexError: | ||
raise ValueError( | ||
f"Invalid service-annotations format. Please use the form key=value." | ||
|
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.
Why not make a dedicated kw? I think that would be cleaner and nicer for code completion in IDEs
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.
Because of the construction of GefyraInstallOptions in lines 66-67. Making registry_url a dedicated keyword pops it from kwargs leading to the option not being respected anymore
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.
Still I think it would be better to make it explicit, just for code completion and typing.
You could add it to the
kwargs
in case a preset is set.