-
Notifications
You must be signed in to change notification settings - Fork 136
Conversation
This builds on #3 |
218c9aa
to
0ef2b99
Compare
This is now rebased. |
- bazel clean && bazel build //... | ||
# We have no Bazel-based testing currently. | ||
# Check that all of our tests pass | ||
- bazel clean && bazel test //... |
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 noticed we're doing this in a few other repos, but is there any reason to clean again here between the build and the test?
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.
Mostly just that I started from a copy/paste of the line above. :)
I'm happy to change if you feel strongly.
@@ -20,14 +20,28 @@ Add the following to your `WORKSPACE` file to add the necessary external depende | |||
|
|||
```python | |||
git_repository( | |||
name = "io_bazel_rules_docker", | |||
commit = "{HEAD}", |
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.
Why are we using {HEAD} here? This won't work when people copy/paste.
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 added this when going through the OSS process because I didn't have a commit yet. The "can't copy/paste" is somewhat by design for now, I think that once we cut a first release we should update this to reference releases.
|
||
message FooReply { | ||
string message = 1; | ||
} |
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.
nit: newline
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.
|
||
|
||
def Resolve(input, tag_to_digest): | ||
"""Translate tag references within the input yaml into digests.""" |
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.
So the idea is that all image references in a yaml get resolved into a digest? Is there a way to specify just a subset of them to resolve? I guess it's not a big deal, but I'm thinking of cases where users specify sidecar containers maintained by other teams/projects.
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.
Yeah, it resolves everything. Once I add images={}
it may make sense to also add an option that will tell this to only resolve published images, but I think my preference is just that: opt out.
k8s/resolver.py
Outdated
with open(args.template, 'r') as f: | ||
inputs = f.read() | ||
|
||
print _DOCUMENT_DELIMITER.join([ |
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.
print function vs. statement?
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.
Old habit. Don't tell Doug :)
…run. This change introduces a `resolver` tool that walks the yaml of a K8s object and resolves things shaped like fully-qualified Docker tags to digests. The current version only operates on remote registry objects, no local artifacts are published as a part of resolution. This change also introduces the first part of our first sample app to use as mock test data for our resolver unit testing.
Should be RFAL. thanks for the feedback. |
], | ||
) | ||
|
||
par_binary( |
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.
It would probably make sense for this to be a standalone tool packaged as part of the containerregistry releases. But this LGTM for now.
No description provided.