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

fix(glide): use kubeapp interfaces for Kubernetes client operations #51

Merged
merged 5 commits into from
May 19, 2016

Conversation

arschles
Copy link
Member

@arschles arschles commented May 18, 2016

Some functions of the code in the data package were creating new Kubernetes clients, using them, and passing them elsewhere. This PR makes a few changes to eliminate that pattern:

  • Creates a single Kubernetes client in the main func
  • Makes the functions that require Kubernetes client functionality accept one or more interfaces under the github.com/arschles/kubeapp/api package, and use those
    • This change allows for unit tests to provide an in-memory implementation to test the behavior of these functions without any externally running services. This change allows us to implement the first bullet point in the description of meta: tests #38:

      We don't currently have a mock/fixture interface for a k8s cluster to test, for example, data.ClusterIDFromPersistentStorage.Get() against. We should consider some way to achieve a functional test of business logic which depends on a k8s abstraction.
      

Contributes to #38

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @jackfrancis and @mboersma to be potential reviewers

@arschles arschles added this to the v2.0-rc1 milestone May 18, 2016
@arschles arschles self-assigned this May 18, 2016
@arschles arschles changed the title fix(glide): add kubeapp dependency fix(glide): use kubeapp interfaces for Kubernetes client operations May 18, 2016
Aaron Schlesinger added 3 commits May 18, 2016 13:55
this dependency will be used for mocking the k8s clients in unit tests
All code that requires a Kubernetes client to interact with secrets now
takes in a KubeSecretGetterCreator - an interface that composes
(github.com/arschles/kubeapp/api/secret).{Getter, Creator} - and the
real Kubernetes client is passed down the stack, as it’s an
implementation of that interface.

Tests are still broken as of this commit, fixes forthcoming.
this commit also modifies various structs in the data package to store
the secret creator/getter inside the struct, instead of changing the
interface definition
pass implementations in prod and test code
@arschles
Copy link
Member Author

I'll augment @mention-bot by guessing that @vdice might be interested in this PR as well 😄

}
deisSecrets := kubeClient.Secrets(deisNamespace)
secret, err := deisSecrets.Get(wfmSecretName)
// deisSecrets := kubeClient.Secrets(deisNamespace)
Copy link
Member

Choose a reason for hiding this comment

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

looks like this commented code can be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

right you are. fixed in bea47fe

@vdice vdice added the LGTM1 label May 18, 2016
@arschles arschles merged commit 86ef470 into deis:master May 19, 2016
@arschles arschles deleted the k8s-unit branch May 19, 2016 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants