[19.03 backport] Dynamically register kubernetes context store endpoint type. - #1907
Merged
thaJeztah merged 8 commits intoMay 27, 2019
Merged
Conversation
added 7 commits
May 24, 2019 15:53
Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit d84e278) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
I'm about to refactor the code which includes the Kubernetes support in a way which relies on something vendoring `./cli/context/kubernetes/` in order to trigger the inclusion of support for the Kubernetes endpoint in the final binary. In practice anything which is interested in Kubernetes must import that package (e.g. `./cli/command/context.list` does for the `EndpointFromContext` function). However if it was somehow possible to build without that import then the `KUBERNETES ENDPOINT` column would be mysteriously empty. Out of an abundance of caution add a specific check on the final binary. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit d5d693a) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
This is a yet unused and the default set remains the same, no expected functional change. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit 087c3f7) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Unused for now. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit 4f14c49) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Previously an endpoint registered using `RegisterDefaultStoreEndpoints` would not be taken into consideration by `resolveDefaultContext` and so could not provide any details. Resolve this by passing a `store.Config` to `resolveDefaultContext` and using it to iterate over all registered endpoints. Any endpoint can ensure that their type implements the new `EndpointDefaultResolver` in order to provide a default. The Docker and Kubernetes endpoints are special cased, shortly the Kubernetes one will be refactored to be dynamically registered. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit 1433e27) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
These are needed by any dynamically registered (via `RegisterDefaultStoreEndpoints`) endpoint type to write a useful/sensible unit test. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit f820766) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
This removes the need for the core context code to import `github.com/docker/cli/cli/context/kubernetes` which in turn reduces the transitive import tree in this file to not pull in all of Kubernetes. Note that this means that any calling code which is interested in the kubernetes endpoint must import `github.com/docker/cli/cli/context/kubernetes` itself somewhere in order to trigger the dynamic registration. In practice anything which is interested in Kubernetes must import that package (e.g. `./cli/command/context.list` does for the `EndpointFromContext` function) to do anything useful, so this restriction is not too onerous. As a special case a small amount of Kubernetes related logic remains in `ResolveDefaultContext` to handle error handling when the stack orchestrator includes Kubernetes. In order to avoid a circular import loop this hardcodes the kube endpoint name. Similarly to avoid an import loop the existing `TestDefaultContextInitializer` cannot continue to unit test for the Kubernetes case, so that aspect of the test is carved off into a very similar test in the kubernetes context package. Lastly, note that the kubernetes endpoint is now modifiable via `WithContextEndpointType`. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit 520be05) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
|
Please sign your commits following these rules: $ git clone -b "19.03_backport-reduce-vendoring-impact2" git@github.com:silvin-lubecki/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842359068512
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 -fAmending updates the existing PR. You DO NOT need to open a new one. |
Codecov Report
@@ Coverage Diff @@
## 19.03 #1907 +/- ##
==========================================
- Coverage 56.72% 56.67% -0.05%
==========================================
Files 309 309
Lines 21678 21707 +29
==========================================
+ Hits 12297 12303 +6
- Misses 8483 8505 +22
- Partials 898 899 +1 |
Member
|
@silvin-lubecki can you try force-pushing again? I think the |
This is less of a layering violation and removes some ugly hardcoded `"kubernetes"` strings which were needed to avoid an import loop. Signed-off-by: Ian Campbell <ijc@docker.com> (cherry picked from commit c455193) Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
silvin-lubecki
force-pushed
the
19.03_backport-reduce-vendoring-impact2
branch
from
May 24, 2019 14:50
c73ccda to
a720cf5
Compare
albers
removed their request for review
May 24, 2019 20:09
Collaborator
|
As the PR does not contain changes to bash completion any more, I removed myself from the reviewers. |
This file contains hidden or 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
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.
Backport of #1890 for 19.03
Further reduce amount of transitive vendoring required by vendorers and builds on (includes) #1887. With this my private project no longer vendors anything from k8s.io. diffstat (in addition to #1887, not cumulative) is:
And it dropped transitive vendorings of
for a ~2M reduction in my vendor dir (which is still 27M, but is now dominated by
golang.org/x/*(mostlysys, buttexttoo),github.com/containerd/containerd,github.com/docker/docker).There's a few things I'm unsure of:
EndpointDefaultResolver interfacebeing implemented by the concrete endpointmeta types feels a bit iffy. Might be better to expandNamedTypeGetterto have an additional method alongside thetypeGetter.ResolveDefaultContextis rather gross (a layering violation of sorts). I justify it (sort of) in that while the kubernetes endpoint is notionally optional there is a coupling via thestackOrchestratoroptions which means the core needs to be somewhat aware. At first I had it where it would pass thestackOrchestratoras an argument tokubernetes.ResolveDefault, I wasn't terribly happy with that either but maybe it is better?ClitoResolveDefaultContextnot astore.Configand usecli.contextStoreConfig(andcli.configFile) butNewAPIClientFromFlagsdoesn't have a cli to use.DefaultContextStoreConfig()andResolveDefaultContext()just for use in unit tests but I couldn't see another way. Similarly/Relatedly I'd have preferred not to need to splitTestDefaultContextInitializerinto a kube part, but couldn't avoid it while also avoiding the circular imports.