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

Annotate "stack" commands to be "swarm" and "kubernetes" #804

Merged
merged 1 commit into from Jan 26, 2018

Conversation

@thaJeztah
Copy link
Member

commented Jan 11, 2018

The stack commands require either "swarm" or "kubernetes"

- Description for the changelog

ping @dnephin @vdemeester @mistyhacks

cmd.Annotations = map[string]string{"experimental": "", "version": "1.25"}
cmd.Annotations = map[string]string{
"experimental": "",
"swarm": "",

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jan 11, 2018

Author Member

I wasn't sure if I should add "kubernetes" for the top-level docker deploy; it's still marked "experimental" (and I think we should consider removing it)

This comment has been minimized.

Copy link
@vdemeester

vdemeester Jan 12, 2018

Member

Yes, I think we should remove the top level one (and thus we don't need to annotate it)

@codecov-io

This comment has been minimized.

Copy link

commented Jan 11, 2018

Codecov Report

Merging #804 into master will decrease coverage by 0.01%.
The diff coverage is 19.04%.

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
- Coverage   52.96%   52.95%   -0.02%     
==========================================
  Files         244      244              
  Lines       15828    15839      +11     
==========================================
+ Hits         8383     8387       +4     
- Misses       6891     6898       +7     
  Partials      554      554
@mistyhacks
Copy link
Contributor

left a comment

LGTM, needs to be cherry-picked definitely.

@vdemeester
Copy link
Member

left a comment

LGTM 🐒

cmd.Annotations = map[string]string{"experimental": "", "version": "1.25"}
cmd.Annotations = map[string]string{
"experimental": "",
"swarm": "",

This comment has been minimized.

Copy link
@vdemeester

vdemeester Jan 12, 2018

Member

Yes, I think we should remove the top level one (and thus we don't need to annotate it)

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

ping @dnephin okay to merge?

@dnephin

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

there is a test failure, let me try to rerun

@dnephin

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

@thaJeztah the e2e test seems to be failing consistently. Can you take a look? I think it's a real failure, doesn't look like a flake.

@dnephin

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

I think the logic for disabling kubernetes doesn't account for swarm also being set ?

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

oh, interesting one; I'd have to check where/how it's used there 😓

@vdemeester

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

@dnephin @thaJeztah right.. the code that checks for swarm/kubernetes is pretty naive and doesn't handle something that would support both (i.e. it make the assumption that it would have none of both…). That needs to change 😅

@vdemeester

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go
index 74d8447f..cd5be656 100644
--- a/cmd/docker/docker.go
+++ b/cmd/docker/docker.go
@@ -277,10 +277,12 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
                        if _, ok := f.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
                                errs = append(errs, fmt.Sprintf("\"--%s\" is only supported when experimental cli features are enabled", f.Name))
                        }
-                       if _, ok := f.Annotations["kubernetes"]; ok && !hasKubernetes {
+                       _, isKubernetesAnnotated := f.Annotations["kubernetes"]
+                       _, isSwarmAnnotated := f.Annotations["swarm"]
+                       if isKubernetesAnnotated && !isSwarmAnnotated && !hasKubernetes {
                                errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with kubernetes features enabled", f.Name))
                        }
-                       if _, ok := f.Annotations["swarm"]; ok && hasKubernetes {
+                       if isSwarmAnnotated && !isKubernetesAnnotated && hasKubernetes {
                                errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with swarm features enabled", f.Name))
                        }
                }
@@ -309,10 +311,12 @@ func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error {
                if _, ok := curr.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
                        return fmt.Errorf("%s is only supported when experimental cli features are enabled", cmd.CommandPath())
                }
-               if _, ok := curr.Annotations["kubernetes"]; ok && !hasKubernetes {
+               _, isKubernetesAnnotated := curr.Annotations["kubernetes"]
+               _, isSwarmAnnotated := curr.Annotations["swarm"]
+               if isKubernetesAnnotated && !isSwarmAnnotated && !hasKubernetes {
                        return fmt.Errorf("%s is only supported on a Docker cli with kubernetes features enabled", cmd.CommandPath())
                }
-               if _, ok := curr.Annotations["swarm"]; ok && hasKubernetes {
+               if isSwarmAnnotated && !isKubernetesAnnotated && hasKubernetes {
                        return fmt.Errorf("%s is only supported on a Docker cli with swarm features enabled", cmd.CommandPath())
                }
        }

Something like the above should fix it 👼

@thaJeztah thaJeztah force-pushed the thaJeztah:more-annotations branch from 294e1de to c216da9 Jan 22, 2018

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

Thanks! Added that change 👍

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

oh, we may need something similar for areSubcommandsSupported(), let me check if that's indeed the case

Annotate "stack" commands to be "swarm" and "kubernetes"
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:more-annotations branch from c216da9 to 93c36eb Jan 22, 2018

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

We should rewrite that logic a bit; it's quite confusing because !hasKubernetes means no orchestrator set means use default means swarm; we can do so in a follow up, as it may require some changes in other locations

@vdemeester

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

ping @dnephin 👼

@vdemeester vdemeester added this to the 18.02.0 milestone Jan 26, 2018

@dnephin
Copy link
Collaborator

left a comment

LGTM

This will need some cleanup if we ever add a third option, but hopefully that wont happen for a while.

@dnephin dnephin merged commit a46fa07 into docker:master Jan 26, 2018

8 of 9 checks passed

codecov/patch 19.04% of diff hit (target 50%)
Details
ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/project 52.95% (-0.02%) compared to f252944
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@thaJeztah thaJeztah deleted the thaJeztah:more-annotations branch Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.