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

Always populate TemplateParams 'Object' #249

Merged
merged 5 commits into from
Aug 28, 2019

Conversation

vkamra
Copy link

@vkamra vkamra commented Aug 27, 2019

Change Overview

This commit modifies template params creation to always
populate the Object field with the unstructured representation
of the Kubernetes object that is the target of the actionset

This allows writing generic blueprints that can be re-used
across different types of Kubernetes resources and to reference
fields from within the object spec that we do not hoist into
TemplateParams e.g. .Object.metadata.labels.<foo>

The change described is contained to param.go and param_test.go.
The rest of the changes are to the kube pkg to allow testing with
a fake dynamic client.

Pull request type

Please check the type of change your PR introduces:

  • Work in Progress
  • Refactoring (no functional changes, no api changes)
  • Trival/Minor
  • Bugfix
  • Feature
  • Documentation

Issues

Test Plan

  • Manual
  • Unit test
  • E2E
$ go test -check.vv
...
OK: 14 passed
PASS
ok  	github.com/kanisterio/kanister/pkg/param	103.201s

@vkamra vkamra requested review from tdmanv and pavannd1 August 27, 2019 23:54
@vkamra
Copy link
Author

vkamra commented Aug 27, 2019

@tdmanv @pavannd1 - reviewing the commits individually might be useful.

//return cli.Resource(resource).Namespace(namespace).List(metav1.ListOptions{})
return cli.Resource(resource).List(metav1.ListOptions{})
// ListUnstructuredObjectWithCli returns the referenced API objects as a map[string]interface{} using the specified CLI
// TODO: deprecate `ListUnstructuredObject`
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -543,6 +564,9 @@ func (s *ParamsSuite) TestPhaseParams(c *C) {
c.Assert(err, IsNil)

crCli := crfake.NewSimpleClientset()
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Create this closer to its use.

Suggested change
crCli := crfake.NewSimpleClientset()

@@ -543,6 +564,9 @@ func (s *ParamsSuite) TestPhaseParams(c *C) {
c.Assert(err, IsNil)

crCli := crfake.NewSimpleClientset()
pvc, err := s.cli.CoreV1().PersistentVolumeClaims(s.namespace).Get(s.pvc, metav1.GetOptions{})
c.Assert(err, IsNil)
dynCli := s.getDynamicClient(c, pvc)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
dynCli := s.getDynamicClient(c, pvc)
dynCli := s.getDynamicClient(c, pvc)
crCli := crfake.NewSimpleClientset()

@vkamra vkamra merged commit da43bcd into master Aug 28, 2019
@vkamra vkamra deleted the always_set_object_in_templateparams branch August 28, 2019 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants