Skip to content
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

windows: Use stdtime for hcsshimtypes.ProcessDetails.CreatedAt #1649

Merged
merged 1 commit into from Oct 17, 2017

Conversation

mlaventure
Copy link
Contributor

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--

/cc @jessvalarezo

@crosbymichael
Copy link
Member

rebase?

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure
Copy link
Contributor Author

rebased

@crosbymichael
Copy link
Member

rebased!

@codecov-io
Copy link

Codecov Report

Merging #1649 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1649   +/-   ##
=======================================
  Coverage   49.06%   49.06%           
=======================================
  Files          27       27           
  Lines        4080     4080           
=======================================
  Hits         2002     2002           
  Misses       1663     1663           
  Partials      415      415

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5fe56...e7ea7b5. Read the comment docs.

@crosbymichael
Copy link
Member

LGTM

@@ -16,7 +16,7 @@ message CreateOptions {
// ProcessDetails is made of the same fields as found in hcsshim.ProcessListItem
message ProcessDetails {
string image_name = 1;
google.protobuf.Timestamp created_at = 2;
google.protobuf.Timestamp created_at = 2 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you figure out that this needed to be added?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and is the conversion from ProcessListItem type time.Time to CreatedAt type google.protobuf.Timestamp now done for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what we use in the other proto files, it indeed permits us not to deal with the google.protobuf.Timestamp conversion as it is done by the generated code now.

One of the advantages of gogoproto. I think we have @stevvooe to thank for using these options originally.

Copy link
Member

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevvooe stevvooe merged commit 3c89aaf into containerd:master Oct 17, 2017
@mlaventure mlaventure deleted the win-procdetaisl-use-stdtime branch October 17, 2017 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants