Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Store context type in metadata to make retrocompatibility with previous contexts easier (potentially switching back and forth) #200

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

gtardif
Copy link
Contributor

@gtardif gtardif commented Jun 10, 2020

What I did

  • context type is now stored in context metadata, not added at the root of context json storage.

Related issue
Needed to ensure backward compatibility if users switch back and forth, to avoid loosing the type info.
Also required to ensure context sync between HyperV and WSL2 environments, allowing to use the /docker/cli code to read/write contexts. cc @simonferquel

(not mandatory) A picture of a cute animal, if possible in relation with what you did
image

@gtardif gtardif requested a review from rumpl June 10, 2020 12:19
// ContextMetadata is represtentation of the data we put in a context
// metadata
type ContextMetadata struct {
Type string `json:",omitempty"`

Choose a reason for hiding this comment

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

This might provoke the same issue in the future as what we had in the previous CLI. See docker/cli#2572 for the classic CLI fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thx !

…us contexts easier (potentially switching back and forth)
@gtardif gtardif force-pushed the context_store_type branch from 97d4bb1 to e768268 Compare June 10, 2020 16:18
"encoding/json"
"testing"

. "github.com/onsi/gomega"
Copy link
Contributor

@chris-crone chris-crone Jun 11, 2020

Choose a reason for hiding this comment

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

nit: We should decide as a team if we want to use gomega or testify/{require,assert}. Not at all urgent but a bit jarring to have two different libraries depending on where we are in the code

Copy link
Member

Choose a reason for hiding this comment

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

We also have gotest.tools, so there's three now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.. gotest.tools doesn't have all the functionality we need. e.g.: Test suites with setup and teardown functions. It also doesn't have something like testify's require which continues testing even if a failure is encountered.

Copy link
Contributor

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

An e2e test that tests the backward compatibility would be good in a follow up

@gtardif gtardif merged commit 93623dc into master Jun 11, 2020
@gtardif gtardif deleted the context_store_type branch June 11, 2020 11:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants