-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Any plans to add providers for writing rules that build on-top of rules_oci? #279
Comments
Usually, our response to "adding something new" to the ruleset is why do you think you need it to accomplish what you are trying to do? |
@thesayyn In order to port our bazel rule, we need access to the digest. From https://github.com/bazel-contrib/rules_oci/blob/main/oci/private/image.bzl#L172 it looks like there are no additonal outputs to obtain it. |
Is this what would you like to obtain? #60 |
I think we should add an OutputGroupInfo so you can request the "digest" output file, but we wouldn't have to create any new Provider types to satisfy this FR. Sound right? |
Thanks! That seems to work for now. |
Does #346 fix this for you? |
Maybe I can chime in my experience here: The additional label with the digest file is going to be very hard to compose for downstream rules (need to recalculate a label, so forced to use a macro, not configurable, etc.). So far, I've just resorted to read the oci-layout index.json directly. This is through a well specified standard. Downsides are:
|
oci_* rules will always have a single image in the index.json so that's not really a concern here.
I believe it could be gathered with a simple |
That is correct, but only under the assumption that users always pass the correct rules forward: Without a provider, all there is to validate at analysis time is that the output is a single tree artifact. For any more serious ruleset, I would not be comfortable just assuming the build user has wired everything correctly (for comparison: with a provider, I only need to trust the rule implementor). |
Another datapoint: Turns out I needed the config digest, not the manifest digest. IIUC with the OIC layout it is infeasible to predict where it is going to end up (since it is content addressable). So to use it, the call-site needs to handle OIC layout anyways. |
I don't see the point of adding a provider, just for the sake of validation. Adding a provider is the wrong tool for validation. A custom rule can still produce an invalid oci-layout and add the provider. And this is separate concern than adding a provider to rules_oci. That'll probably solved via https://bazel.build/extending/rules#validation_actions. We are just waiting to drop bazel 5 support. |
We could add a validation action now, and just create it conditional on using Bazel 6. I think this is now a different feature request? |
Agreed. Regarding provider and validation, I think I wasn't very clear. Apologies. I'll try to elaborate. Consider the following, broken build. create_dir(
# some rule that creates an output directory.
name = "foo",
...
)
oci_image(
# some arbitrary image.
name = "bar",
...
)
my_rule_using_an_image(
name = "baz",
image = "foo", # this should be bar
) The user made a mistake here and passed the wrong label/target to I was looking at this situation from the perspective of the implementer of With a providerIn the analysis phase, bazel itself will not accept the
Without a providerAnalysis phase
Execution phase
While this gives the user appropriate feedback:
DiscussionIf we look at the two scenarios above in the context of simply retrieving the manifest digest, in the second scenario (without a provider), the code required for the retrieval will be overshadowed by the code required to prevent users from making mistakes (which IMHO is paramount). I guess my informal use of the term "validate" has caused some confusion. IIUC to solve the issue I've outlined, a validation action is not useful (or I have misunderstood something about validation actions 🤷 ). Whether or not all of this is sufficient reason to actually add a provider to rules_oci, and/or whether a provider is the right tool to solve the outlined issue is not 100% clear to me. I just wanted to make sure we're on the same page RE the use cases I had in mind. |
Validation actions do what providers would do for "validation" but at runtime. I'd also argue that adding a provider to get bazel to yell users with a message that looks like "target :x does not provide OCIImage provider." is not as useful as one would think. Adding a provider for this use would simply prevent anyone from using things like If we added providers for this, it would simply prevented us from doing things like this. |
That's an excellent example indeed. I guess this basically boils down to the nominal / structural subtyping discussion and to some extent the static / dynamic subtyping discussion. In any case, it feels like at this point, consistency with the bazel ecosystem is significantly more important than doing "the right thing". I do not feel I have a sufficiently large understanding of how the ecosystem handles this to have any opinion TBH. So from my personal POV, feel free to close this issue. |
You are correct, but this isn't because we are trying specifically do the right thing thus diverging from the community. You can find plenty of rulesets that don't have a custom provider. oci_image is in sense is like pkg_tar, you feed it a bunch files and it packages them into a folder (as opposed to a tar file) that is understood by most container tooling.
I agree. |
We're trying to port from rules_docker to rules_oci (see googlecloudrobotics/core#130), but have trouble finding a good replacement for:
load("@io_bazel_rules_docker//container:providers.bzl", "ImageInfo", "ImportInfo")
see https://github.com/googlecloudrobotics/core/blob/main/bazel/app_chart.bzl#L1
The text was updated successfully, but these errors were encountered: