Implement registry_credential for basic authentication #531
Conversation
| type = ctx.attr.type, | ||
| )] | ||
|
|
||
| registry_credential = rule( |
There was a problem hiding this comment.
I'm not sure if this should be a repository rule or a skylark rule as it is now as this is a bit problematic.
Specifically, there are two issues:
- this rule will not work for container_pull, as container_pull is a repository rule and cannot access outputs of a skylark rule
- env vars like this one should be read by repository rules, not skylark rules, particularly because if the user changes the values of the env variables, Bazel will not regenerate the outputs unless you do a Bazel clean
However, I'm also not sure this should be a repository_rule. Specifically because:
- repo rules cannot have providers, so the elegant solution that you have to avoid users from checking in their secret values cannot be used here (i.e., the repo rule will need to produce as output the docker config file, and the container_push rule will need to receive a file; in the case of container_pull the logic to read the env var and create the file will need to be reused, as repo rules cannot depend on other repo rules afaik)
Thoughts?
There was a problem hiding this comment.
You're right about container_pull would need a different mechanism. I'll have to think about how to reconcile this.
Regarding changing of environment variables. The implementation of --action_env includes the env vars into the SkyKey (or whatever the action hash is called), so in fact it does re-run the action if you change the value of the environment variable.
There was a problem hiding this comment.
You're right about env vars, did not remember the action key does include those values.
let me know what you think about container_pull
| registry_credential = rule( | ||
| implementation = _registry_credential_impl, | ||
| attrs = { | ||
| "user_env": attr.string( |
There was a problem hiding this comment.
if this was a repo rule, env variables to be read would be declared in a way that Bazel knows to regenerate outputs if values change.
example:
https://github.com/bazelbuild/bazel/blob/master/tools/cpp/cc_configure.bzl#L59
There was a problem hiding this comment.
See comment above. It doesn't have to be a repository rule.
| config_json = ctx.actions.declare_file("%s/.docker/config.json" % credential.name) | ||
| # NOTE: ctx.actions.write does not work here as we need the | ||
| # use_default_shell_env feature of the run_shell action. | ||
| ctx.actions.run_shell( |
There was a problem hiding this comment.
you should use a template file + expand_template to avoid using shell to create a file (which would only work on environments that have bash) https://docs.bazel.build/versions/master/skylark/lib/actions.html#expand_template
There was a problem hiding this comment.
Yes that was my initial thought. However, in order to use --action_env you MUST use a run or run_shell action, which has the use_default_shell_env feature. (See NOTE:)
There was a problem hiding this comment.
I really dislike having this code here. We're in process of fixing any instances of usage of tools that will block these rules from working with remote execution or with windows. The way to avoid this hard restriction (--action_env you MUST use a run or run_shell), I think we could use ctx.var (https://docs.bazel.build/versions/master/skylark/lib/ctx.html#var). The main problem with that is that we would not get the env var to be part of the action key. In any case, it should at least be possible to use templates to generate the file with the contents you are generating here using format, and then use run_shell to execute the file produced by the template function.
However, I think we need to hash out the issue about how to do this for container_pull and that will likely require change how were doing this with --action_env. I don't want there to be two different paths to achieve the same thing for container_pull/push.
|
Closing this PR as this feature will be implemented in #531 via toolchain rules |
|
SGTM |
This is a WIP/prototype for #526 that works in conjunction with google/containerregistry#112.