search

bazel-contrib / rules_oci

Bazel rules for building OCI containers
Apache License 2.0
305 stars 159 forks source link

fix: pass global args to all crane invocations #709

Closed ahans closed 1 month ago

ahans commented 1 month ago

This fixes #527. Please let me know if I missed anything. Thanks!

thesayyn commented 1 month ago

this change looks like a breaking change, before args are passed only to crane push, but now args are passed to every subcommand.

principled fix for this is to only pass global flags to all by plucking them out of args first.

ahans commented 1 month ago

this change looks like a breaking change, before args are passed only to crane push, but now args are passed to every subcommand.

principled fix for this is to only pass global flags to all by plucking them out of args first.

I see, so you only want to pass on the following flags?

Global Flags:
      --allow-nondistributable-artifacts   Allow pushing non-distributable (foreign) layers
      --insecure                           Allow image references to be fetched without TLS
      --platform platform                  Specifies the platform in the form os/arch[/variant][:osversion] (e.g. linux/amd64). (default all)
  -v, --verbose                            Enable debug logs

TBH, I found it a bit surprising that one is supposed to use args = ["--insecure"] to support repositories whose certificate can't be validated. I would prefer an explicit flag such as insecure_repository as we had with rules_docker. I'm not sure if it's such a good idea to allow the user to pass flags to crane. Isn't usage of crane an implementation detail here? Anyway, I'm happy to implement the approach above that filters out global flags, but would like to understand the motivation for this design first. What are other usecases for passing in args?

thesayyn commented 1 month ago

args something bazel adds support for runnable rules by default and there is no way to disable it. Also, crane is not implementation detail for oci_push unfortunately, we have supported passing arbitrary args to oci_push since the beginning of rules_oci.

thesayyn commented 1 month ago

I would prefer an explicit flag such as insecure_repository as we had with rules_docker

Not every flag deserves its own attribute, if it translates 1:1 to the underlying tool, if we did support something like this then there would be two different way of getting the same outcome, eg: args and insecure_repository and we'd have to maintain both and resolve conflicts if args = ["--insecure"] and insecure = True|False. I'd rather keep it simple and just pass all the global flags to all crane invocations.

ahans commented 1 month ago

Ok, I think disabling args support would be the cleanest solution here. But since that's not possible and we want to maintain backwards compatibility, allowing users to pass everything via args makes sense. Thanks for the explanation, that helps a lot!

I updated the PR now to put all global flags into a GLOBAL_FLAGS list and pass that separately to each crane invocation. Since --verbose (or -v) is just one of those special global flags, I removed the special handling for that. I hope it's better now! Looking forward to your review!

ahans commented 1 month ago

Anything I can do about the test failure? Looks like some infrastructure issue ...