Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Add Version information to plugin APIs #318

Merged
merged 5 commits into from
Dec 5, 2016

Conversation

wfarner
Copy link
Contributor

@wfarner wfarner commented Dec 3, 2016

Publishing just to share the approach taken so far. Feedback welcome.

Closes #309

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "api-version" git@github.com:wfarner/infrakit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354112880
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@wfarner wfarner changed the title [NOT READY TO MERGE] Add Version information to plugin APIs Add Version information to plugin APIs Dec 4, 2016
@codecov-io
Copy link

codecov-io commented Dec 4, 2016

Current coverage is 67.89% (diff: 91.11%)

Merging #318 into master will increase coverage by 15.60%

@@             master       #318   diff @@
==========================================
  Files            12         30    +18   
  Lines           610       1545   +935   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            319       1049   +730   
- Misses          246        396   +150   
- Partials         45        100    +55   

Powered by Codecov. Last update a1e94af...a9f948f


handshakeResult *handshakeResult

// lock guards handshakeComplete
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment - do you mean handshakeResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, thanks. Fixing.

Signed-off-by: Bill Farner <bill@docker.com>
Signed-off-by: Bill Farner <bill@docker.com>
Signed-off-by: Bill Farner <bill@docker.com>
@@ -0,0 +1,16 @@
package spi
Copy link
Contributor

@chungers chungers Dec 5, 2016

Choose a reason for hiding this comment

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

As a nit -- would you mind rename API to something like Interface? API looks jarring when there are names like APIsRequest and APIsResponse or APIs().

Also for the method names APIs(), Implements() looks a little nicer to me. It also matches how Docker engine does activation with the field Implements: https://docs.docker.com/engine/extend/plugin_api/#/pluginactivate

So how about

type Interface struct {
   Name string
   Version string
}

type Plugin interface {
    Implements() ([]Interface, error)
}

Then the RPC response can be named HandshakeResponse and it will look like

{
     "Implements" : [ ....]
     "Interfaces" : [  /* list of interface descriptions like example input / output via reflection */ ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinions here, the names you gave sound fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the flexibility. I agonize over naming and shape of characters when they are laid out next to each other.

Signed-off-by: Bill Farner <bill@docker.com>
Copy link
Contributor

@chungers chungers left a comment

Choose a reason for hiding this comment

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

LGTM

@chungers chungers merged commit eab86f0 into docker-archive:master Dec 5, 2016
@wfarner wfarner deleted the api-version branch December 5, 2016 21:03
chungers pushed a commit to chungers/infrakit that referenced this pull request Dec 7, 2016
…int /metaz to return version and api information

Signed-off-by: David Chung <david.chung@docker.com>
chungers pushed a commit to chungers/infrakit that referenced this pull request Sep 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants