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

prepare stage: introduce --no-auto-prune command-line flag #42

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

edigaryev
Copy link
Contributor

@edigaryev edigaryev commented Nov 4, 2023

GitLab Runner prefixes all external environment variables passed to the custom runner with CUSTOM_ENV_*, so
#41 (comment) wouldn't work.

Since TART_NO_AUTO_PRUNE might influence behavior for other tasks, it makes sense to add it as a command-line flag, instead of an environment variable.

@edigaryev edigaryev requested a review from fkorotkov November 4, 2023 10:56
@@ -40,6 +41,9 @@ func NewCommand() *cobra.Command {
"Override default image memory configuration (size in megabytes or \"auto\")")
command.PersistentFlags().StringArrayVar(&customDirectoryMounts, "dir", []string{},
"\"--dir\" arguments to pass to \"tart run\", can be specified multiple times")
command.PersistentFlags().BoolVar(&noAutoPrune, "no-auto-prune", false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it automatically support --no prefix for bool flags? Maybe invert it and default to true to always pool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't it automatically support --no prefix for bool flags?

I don't think so: spf13/pflag#214.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of --auto-prune flag with a a default to true? Potential usage like --no-auto-prune=false looks weird to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See bebf869.

@edigaryev edigaryev requested a review from fkorotkov November 4, 2023 17:30
@edigaryev edigaryev merged commit 0cc5be4 into main Nov 6, 2023
1 check 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.

2 participants