Skip to content

Handle multiple stack api versions - #899

Merged
thaJeztah merged 1 commit into
docker:masterfrom
silvin-lubecki:handle-multiple-stack-api-versions
Apr 30, 2018
Merged

thaJeztah merged 1 commit into
docker:masterfrom
silvin-lubecki:handle-multiple-stack-api-versions

Conversation

@silvin-lubecki

Copy link
Copy Markdown
Contributor

- What I did
I integrated the new version of the Stack API (v1beta2). The CLI can now communicates with both versions.

⚠️ This PR has a dependency on #898 ⚠️

- How I did it
I added a common interface between v1beta1 and v1beta2 stack clients and one common object stack, so the code in the commands remains the same.

- How to verify it
Execute a docker version command against latest docker for mac, it should output at the end:

 Orchestrator:
   Kubernetes:  v1.9.2
   Stack:       v1beta2

- Description for the changelog

  • Add support for the new Stack API for Kubernetes v1beta2

- A picture of a cute animal (not mandatory but encouraged)

image

@codecov-io

codecov-io commented Feb 22, 2018

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@99bd7ed). Click here to learn what that means.
The diff coverage is 23.17%.

@@            Coverage Diff            @@
##             master     #899   +/-   ##
=========================================
  Coverage          ?   53.51%           
=========================================
  Files             ?      266           
  Lines             ?    16891           
  Branches          ?        0           
=========================================
  Hits              ?     9040           
  Misses            ?     7248           
  Partials          ?      603

@vdemeester vdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • I think we can clean more code. There is quite a few code in the kubernetes api package that are dead code (in terms of client's code) — like all the Clone functions, Discovery, etc..
  • We should re-enable linters of kubernetes package(s)
  • I wonder what is bringing golang-lru and the other new vendoring code 🤔


fileName := path.Base(config.File)
content, err := ioutil.ReadFile(config.File)
if err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • Do we want to group errors ?
  • If a file can't read (but not the first one), a configMap will be created in k8s and not used (orphan) — and will probably cause errors next time we do the docker stack deploy, right ?

I think we should "read all", and if there is no error, "create all". Then, if one of the creation fails, what should we do with the previous one created ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll fix that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I propose to fix this in a follow-up, as we may remove all configmaps and secrets if any error occurs, even while deploying the stack.


fileName := path.Base(secret.File)
content, err := ioutil.ReadFile(secret.File)
if err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same question here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll fix that too.

stacks composev1beta2.StackInterface
}

func (c *KubeCli) newStackV1Beta2() (stackClient, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would rather have func newStackV1Beta2(config *rest.Config, namespace string) … here (not tying it up with KubeCli)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

stacks composev1beta1.StackInterface
}

func (c *KubeCli) newStackV1Beta1() (stackClient, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would rather have func newStackV1Beta1(config *rest.Config, namespace string) … here (not tying it up with KubeCli)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

Comment thread kubernetes/README.md
@@ -1,4 +0,0 @@
# Kubernetes client libraries

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we keep the README ? 👼

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure!

)