-
Notifications
You must be signed in to change notification settings - Fork 136
Conversation
Can one of the admins verify this patch? |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
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.
Generally this LGTM.
Travis will fail because this isn't from a branch on the main repo and so it doesn't hand out the secrets (bad design!).
Generally the process (which I will handle, but FYI) is to merge the PR into a branch of the main repo, then open a second PR to merge it into mainline (which will run integration testing properly).
A few minor comments below.
README.md
Outdated
) | ||
``` | ||
|
||
This script needs to invoke the default resolver (`//k8s:resolver`) with all its |
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 would say that this script may need to invoke the default resolver, if the template may also contain image references.
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.
Done.
@@ -173,7 +173,7 @@ _common_attrs = { | |||
# This is only needed for describe. | |||
"kind": attr.string(), | |||
"image_chroot": attr.string(), | |||
"_resolver": attr.label( | |||
"resolver": attr.label( |
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.
Can you add this to the k8s_object
signature documentation at the bottom of the README.md
?
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.
Done.
Thanks for the review! I addressed the minor comments. Let me know if there's anything else I need to be doing. |
The custom resolver mattmoor@ mentioned seems to be a very workable solution for endpoint resolving (#88). I've made a PoC implementation in http://shortn/_0SyJs3BlK7 (Google-internal link) that uses a custom resolver to resolve endpoint versions after Docker digests were resolved. Could you PTAL?