-
Notifications
You must be signed in to change notification settings - Fork 70
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
Adding metadata to container listing #225
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 11826224650Details
💛 - Coveralls |
containerz/containerz.proto
Outdated
// The container image name to be removed. | ||
string name = 1; | ||
// The container name to be removed. | ||
string instance_name = 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.
We should not just change the field name her -- any textproto instances will no longer unmarshal correctly. Is there a reason that this needs to be changed, or can we make this clearer in the description?
If we consider that we need to change the name, deprecate these field, reserve the tag, and add a new one please.
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.
Ah good point about textprotos, I will revert this change as it is not entirely necessary.
containerz/containerz.proto
Outdated
// Metadata associated to this container. | ||
map<string, string> labels = 5; | ||
|
||
// The computed hash as returned by the container runtime. |
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.
The computed hash of the image?
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.
yes - updated.
Looks like |
Looks like kicking the bazel cache solved that issue -- just the staticcheck one to look at, |
Not sure why the static check is failing here - I regenerated the pb.go files and I still get the error. Does this break for you too or is it just me? |
4fb7f21
to
cd90bd2
Compare
.github/workflows/go.yml
Outdated
@@ -10,3 +10,5 @@ on: | |||
jobs: | |||
go: | |||
uses: openconfig/common-ci/.github/workflows/[email protected] | |||
with: | |||
skip-staticcheck: true |
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.
Let's not do this.
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.
Blocking merge whilst we fix staticcheck in a more robust way.
This commit adds the following: * ListResponse must now return any metadata attached to a container. * ListResponse must now return the container hash. * RemoveContainer should not remove the specified container.
This commit adds the following: