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

feat: add string slice arguments to our parsed_flags library #1852

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

omar711
Copy link
Contributor

@omar711 omar711 commented Nov 27, 2023

Description:

Adds slice strings to our parsed_flags library. This will let us specify flags for the upcoming static ports work, e.g.:

kurtosis run --connect web.http --connect db.postgresql
# or
kurtosis run --connect web.http,db.postgresql

Is this change user facing?

NO

References (if applicable):

@omar711 omar711 requested a review from a team November 27, 2023 14:49
@omar711 omar711 removed the request for review from a team November 27, 2023 16:10
@omar711
Copy link
Contributor Author

omar711 commented Nov 27, 2023

sorry, this isn't fininshed, I'll reopen once set

@omar711 omar711 closed this Nov 27, 2023
@omar711 omar711 reopened this Nov 27, 2023
cobraFlagSet *flag.FlagSet,
) error {

if defaultValueStr != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an assumption in the library that defaults will always derive from strings.

We could look to parse string representations of arrays, or add more genral support e.g. for []string here.

However, my thinking is defaults don't make much sense for a slice flag, so just make it clear it won't work, where somebody does specify a default.

@omar711 omar711 requested a review from a team November 27, 2023 22:36
@adschwartz adschwartz removed the request for review from a team December 7, 2023 14:11
auto-merge was automatically disabled December 15, 2023 15:07

Merge queue setting changed

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.

None yet

1 participant