Drop support for (archived) Compose-on-Kubernetes #3139
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndeloof
force-pushed
the
drop_kubernetes_support
branch
5 times, most recently
from
abb4bd1
to
6077685
Compare
|
Does this mean we should also remove the "kubernetes endpoint" from |
|
@thaJeztah yes indeed |
|
There are still instances of compose-on-kubernetes in the source even with this patch: |
|
💯 |
This was referenced Dec 10, 2021
Merged
ndeloof
force-pushed
the
drop_kubernetes_support
branch
from
6077685
to
1926be1
Compare
Codecov Report
@@ Coverage Diff @@
## master #3139 +/- ##
==========================================
+ Coverage 57.33% 58.29% +0.95%
==========================================
Files 304 287 -17
Lines 26379 24142 -2237
==========================================
- Hits 15124 14073 -1051
+ Misses 10329 9210 -1119
+ Partials 926 859 -67 |
thaJeztah
requested changes
Feb 22, 2022
There was a problem hiding this comment.
Thanks! Left one comment in-line w.r.t. the test, and some comment about (probably ok as follow-up)
Could you;
- have a quick peek at the test (do we still need that case? if so, should it be in a separate test? or should we remove it?)
- squash the commits; I think the first commit doesn't build (so would disallow doing a
git bisect); it's probably fine to squash all commits to a single one - Looks like a linter isn't happy;
cli/command/context/create.go:6: File is not `goimports`-ed (goimports)
"github.com/sirupsen/logrus"
cli/command/context/update.go:6: File is not `goimports`-ed (goimports)
"github.com/sirupsen/logrus"
cli/command/context/create.go
Outdated
| @@ -107,6 +108,9 @@ func createNewContext(o *CreateOptions, stackOrchestrator command.Orchestrator, | |||
| if dockerTLS != nil { | |||
| contextTLSData.Endpoints[docker.DockerEndpoint] = *dockerTLS | |||
| } | |||
| if len(o.Kubernetes) != 0 { | |||
| logrus.Warn("kubernetes orchestrator is deprecated") | |||
There was a problem hiding this comment.
I wanted to suggest to perhaps just use fmt.Printf() here (as logrus formatting is a bit "meh" on the CLI), but I'm ok with doing in a follow-up;
docker context create --docker host=tcp://localhost:2375 --kubernetes host=http://foo:1234 bla
WARN[0000] kubernetes orchestrator is deprecated
bla
Successfully created context "bla"Perhaps we should generate the warning earlier, or use flags.MarkDeprecated() (which both hides the flag, and prints a deprecation message)
Giving it a quick try what it looks like with flags.MarkDeprecated();
diff --git a/cli/command/context/update.go b/cli/command/context/update.go
index 55f775037..bcf1a6101 100644
--- a/cli/command/context/update.go
+++ b/cli/command/context/update.go
@@ -3,7 +3,6 @@ package context
import (
"bytes"
"fmt"
- "github.com/sirupsen/logrus"
"text/tabwriter"
"github.com/docker/cli/cli"
@@ -60,8 +59,8 @@ func newUpdateCommand(dockerCli command.Cli) *cobra.Command {
flags.SetAnnotation("default-stack-orchestrator", "deprecated", nil)
flags.StringToStringVar(&opts.Docker, "docker", nil, "set the docker endpoint")
flags.StringToStringVar(&opts.Kubernetes, "kubernetes", nil, "set the kubernetes endpoint")
+ flags.MarkDeprecated("kubernetes", "option will be ignored")
flags.SetAnnotation("kubernetes", "kubernetes", nil)
- flags.SetAnnotation("kubernetes", "deprecated", nil)
return cmd
}
@@ -102,9 +101,6 @@ func RunUpdate(cli command.Cli, o *UpdateOptions) error {
c.Endpoints[docker.DockerEndpoint] = dockerEP
tlsDataToReset[docker.DockerEndpoint] = dockerTLS
}
- if len(o.Kubernetes) != 0 {
- logrus.Warn("kubernetes orchestrator is deprecated")
- }
if err := validateEndpointsAndOrchestrator(c); err != nil {
return err
}docker context update --kubernetes host=tcp://foo:1234 bla
Flag --kubernetes has been deprecated, option will be ignored
bla
Successfully updated context "bla"
|
I also had a quick look at what it looks like on this PR; I think there's some follow-up changes we should make First of all, wondering if we should consider removing the docker context ls
NAME DESCRIPTION DOCKER ENDPOINT ORCHESTRATOR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock swarmLooking at docker context create --help
Usage: docker context create [OPTIONS] CONTEXT
Create a context
Docker endpoint config:
NAME DESCRIPTION
from Copy named context's Docker endpoint configuration
host Docker endpoint on which to connect
ca Trust certs signed only by this CA
cert Path to TLS certificate file
key Path to TLS key file
skip-tls-verify Skip TLS certificate validation
Example:
$ docker context create my-context --description "some description" --docker "host=tcp://myserver:2376,ca=~/ca-file,cert=~/cert-file,key=~/key-file"
Options:
--default-stack-orchestrator string Default orchestrator for stack operations to use with this context (swarm|kubernetes|all)
--description string Description of the context
--docker stringToString set the docker endpoint (default [])
--from string create context from a named context
--kubernetes stringToString set the kubernetes endpoint (default [])From the above, I would suggest to:
It looks like there's logic in various locations to (e.g.) normalize Orchestrator, etc. etc. that we should probably remove. Effectively we'll only have 1 orchestrator now (swarmkit; and only if swarmkit is enabled), so perhaps there's more cruft to remove. |
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…text Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
drop_kubernetes_support
branch
3 times, most recently
from
feb7525
to
5e485c1
Compare
ndeloof
force-pushed
the
drop_kubernetes_support
branch
4 times, most recently
from
b93e35e
to
b17830f
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
force-pushed
the
drop_kubernetes_support
branch
from
b17830f
to
193ede9
Compare
|
@thaJeztah squashed and removed support for multiple orchestrator in context store / stack commands |
thaJeztah
approved these changes
Feb 23, 2022
There was a problem hiding this comment.
LGTM, thanks for updating.
I was going through the code, and noticed there's a bunch of locations still referring to the removed options (docs, completion scripts), but let me open a follow-up PR for those changes (possibly there's more, so let's first remove what we have in this PR).
|
@silvin-lubecki this one good to go? |
silvin-lubecki
approved these changes
Feb 24, 2022
There was a problem hiding this comment.
LGTM, but @thaJeztah should we update https://github.com/docker/cli/blob/master/docs/deprecated.md#kubernetes-stack-and-context-support and marked it as removed ?
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Yes! I had some changes already locally as follow-up (didn't want to block the PR 😅), but I see @ndeloof updated it (thanks!); let's merge this when CI completes 👍 |
|
LOL, and of course some failure in CI now (looks like it may be a flaky / race (based on the name of the test); I'll kick CI again |
|
It's green! ✅✅ ✅ ✅ |
This was referenced Feb 24, 2022
Merged
This was referenced Apr 30, 2022
matt9ucci
added a commit
to matt9ucci/DockerCompletion
that referenced
this pull request
Feb 9, 2023
matt9ucci
added a commit
to matt9ucci/DockerCompletion
that referenced
this pull request
Feb 9, 2023
This was referenced Jun 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #2967
closes #3389
(https://docker.atlassian.net/browse/IL-257 for internal tracker)
- What I did
Removed support for Kubernetes orchestrator, as required compose-on-kubernetes backend is archived
- How I did it
Code removal
- How to verify it
isn't this what CI is for ? :P
- Description for the changelog
Removed support for deprecated Compose-on-Kubernetes
- A picture of a cute animal (not mandatory but encouraged)
