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

Renamed Generic Resources to match Moby PR #2292

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

RenaudWasTaken
Copy link
Contributor

Moby PR that matches this feature: moby/moby#33440

Renamed:

  • Str -> NamedResourceSpec
  • Discrete -> DiscreteResourceSpec
  • GenericString -> NamedGenericResource
  • GenericDiscrete -> DiscreteGenericResource

Hopefully this will move the moby PR forward and we can start working on CLI integration, compose integration, swarmkit constraints, resource discovery, ...

Ping @aaronlehmann @dongluochen @aluzzardi
Thanks!

Signed-off-by: Renaud Gaubert rgaubert@nvidia.com

@codecov
Copy link

codecov bot commented Jun 30, 2017

Codecov Report

Merging #2292 into master will decrease coverage by 0.15%.
The diff coverage is 69.38%.

@@            Coverage Diff             @@
##           master    #2292      +/-   ##
==========================================
- Coverage   61.29%   61.14%   -0.16%     
==========================================
  Files         128      128              
  Lines       20549    20549              
==========================================
- Hits        12596    12564      -32     
- Misses       6574     6606      +32     
  Partials     1379     1379

@aaronlehmann
Copy link
Collaborator

LGTM

ping @stevvooe

api/types.proto Outdated
string kind = 1;
string value = 2;
}

message GenericDiscrete {
message DiscreteGenericResource {
string kind = 1;
int64 value = 2;
}

message GenericResource {
oneof resource {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aaronlehmann Is there any reason we would want to use an Any vs oneof here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it would make sense to use Any here. The scheduler needs to understand these so it can perform accounting of resource availability on nodes.

api/types.proto Outdated
@@ -30,20 +30,20 @@ message Annotations {
repeated IndexEntry indices = 4 [(gogoproto.nullable) = false];
}

message GenericString {
message NamedGenericResource {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to document what these are.

@stevvooe
Copy link
Contributor

stevvooe commented Jul 7, 2017

LGTM after docs (its also okay to move on and doc in another PR).

@RenaudWasTaken
Copy link
Contributor Author

Added doc

@aaronlehmann
Copy link
Collaborator

Looks like you need to rerun make generate.

Renamed:
- Str -> NamedResourceSpec
- Discrete -> DiscreteResourceSpec
- GenericString -> NamedGenericResource
- GenericDiscrete -> DiscreteGenericResource

Signed-off-by: Renaud Gaubert <rgaubert@nvidia.com>
@RenaudWasTaken
Copy link
Contributor Author

RenaudWasTaken commented Jul 7, 2017

Ok did not expect comments to need to regenerate :)
In retrospective though it makes sense since they are copied in the generated file.
updated the PR

@aaronlehmann aaronlehmann merged commit 69b4e0b into moby:master Jul 7, 2017
@RenaudWasTaken
Copy link
Contributor Author

RenaudWasTaken commented Jul 7, 2017

I'm looking at the vendoring of the moby project :)

@aaronlehmann it seems to me that the only commit that has an impact on moby is 9cbc2cc

And that I should only need to mirror the changes to the executor and that should be it.

Does that sound right to you ?

@aaronlehmann
Copy link
Collaborator

Sorry about that. Yes, mirroring those changes would be ideal, but if it's nontrivial to implement or test, feel free to pass nil for NodeDescription and let someone else implement that as a followup.

andrewhsu pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 14, 2017
- moby/swarmkit#2266 (support for templating Node.Hostname in docker executor)
- moby/swarmkit#2281 (change restore action on objects to be update, not delete/create)
- moby/swarmkit#2285 (extend watch queue with timeout and size limit)
- moby/swarmkit#2253 (version-aware failure tracking in the scheduler)
- moby/swarmkit#2275 (update containerd and port executor to container client library)
- moby/swarmkit#2292 (rename some generic resources)
- moby/swarmkit#2300 (limit the size of the external CA response)
- moby/swarmkit#2301 (delete global tasks when the node running them is deleted)

Minor cleanups, dependency bumps, and vendoring:
- moby/swarmkit#2271
- moby/swarmkit#2279
- moby/swarmkit#2283
- moby/swarmkit#2282
- moby/swarmkit#2274
- moby/swarmkit#2296 (dependency bump of etcd, go-winio)

Signed-off-by: Ying Li <ying.li@docker.com>
Upstream-commit: 4509a00
Component: engine
silvin-lubecki pushed a commit to silvin-lubecki/engine-extract that referenced this pull request Mar 16, 2020
- moby/swarmkit#2266 (support for templating Node.Hostname in docker executor)
- moby/swarmkit#2281 (change restore action on objects to be update, not delete/create)
- moby/swarmkit#2285 (extend watch queue with timeout and size limit)
- moby/swarmkit#2253 (version-aware failure tracking in the scheduler)
- moby/swarmkit#2275 (update containerd and port executor to container client library)
- moby/swarmkit#2292 (rename some generic resources)
- moby/swarmkit#2300 (limit the size of the external CA response)
- moby/swarmkit#2301 (delete global tasks when the node running them is deleted)

Minor cleanups, dependency bumps, and vendoring:
- moby/swarmkit#2271
- moby/swarmkit#2279
- moby/swarmkit#2283
- moby/swarmkit#2282
- moby/swarmkit#2274
- moby/swarmkit#2296 (dependency bump of etcd, go-winio)

Signed-off-by: Ying Li <ying.li@docker.com>
Upstream-commit: 4509a00
Component: engine
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.

3 participants