-
Notifications
You must be signed in to change notification settings - Fork 503
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
hack: generate vtproto files for buildx #2724
Conversation
Integrates vtproto into buildx. The generated files dockerfile has been modified to copy the buildkit equivalent file to ensure files are laid out in the appropriate way for imports. An import has also been included to change the grpc codec to the version in buildkit that supports vtproto. This will allow buildx to utilize the speed and memory improvements from that. Also updates the gc control options for prune. Signed-off-by: Jonathan A. Sternberg <[email protected]>
flags.BoolVar(&options.verbose, "verbose", false, "Provide a more verbose output") | ||
flags.BoolVarP(&options.force, "force", "f", false, "Do not prompt for confirmation") | ||
|
||
flags.Var(&options.maxStorage, "keep-storage", "Amount of disk space to keep for cache") | ||
flags.Var(&options.reservedSpace, "keep-storage", "Amount of disk space to keep for cache") |
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.
We can mark this hidden I think. MarkDeprecated
already sets hidden.
There is no problem with both using &options.reservedSpace
as target? Maybe we should check this separately and also detect if both flags are set.
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.
MarkDeprecated
makes it hidden too along with a message. I don't believe there's any problem with both being set. The second will just overwrite the first and then they'll get a deprecated message. I don't think we need to detect if both are set because they should have the same behavior.
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 don't think we need to detect if both are set because they should have the same behavior.
But if one flag is set with one value and second with another then it is not correct.
@@ -149,12 +150,13 @@ func pruneCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { | |||
flags := cmd.Flags() | |||
flags.BoolVarP(&options.all, "all", "a", false, "Include internal/frontend images") | |||
flags.Var(&options.filter, "filter", `Provide filter values (e.g., "until=24h")`) | |||
flags.Var(&options.minStorage, "min-storage", "Minimum amount of disk space to keep for cache") |
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.
Do these need a deprecation as well?
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.
No, these were never in a release.
Signed-off-by: Tonis Tiigi <[email protected]>
Integrates vtproto into buildx. The generated files dockerfile has been modified to copy the buildkit equivalent file to ensure files are laid out in the appropriate way for imports.
An import has also been included to change the grpc codec to the version in buildkit that supports vtproto. This will allow buildx to utilize the speed and memory improvements from that.