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

Make child resource creation atomic when creating a k8s stack #1611

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

simonferquel
Copy link
Contributor

- What I did

I made sure that configs and secrets created with the CLI are

  • correctly removed if the stack itself can't be created (validation error,...)
  • have their owner reference set to the stack when the stack is correctly created

- How I did it

  • added a childResource interface, with setOwner and delete functions
  • keep track of all created childResources

- How to verify it

The PR comes with unit tests

@codecov-io
Copy link

codecov-io commented Jan 11, 2019

Codecov Report

Merging #1611 into master will increase coverage by 0.4%.
The diff coverage is 75.53%.

@@            Coverage Diff            @@
##           master    #1611     +/-   ##
=========================================
+ Coverage   55.16%   55.56%   +0.4%     
=========================================
  Files         301      301             
  Lines       20384    20451     +67     
=========================================
+ Hits        11244    11363    +119     
+ Misses       8335     8270     -65     
- Partials      805      818     +13

@simonferquel simonferquel force-pushed the stack-children-atomic branch 7 times, most recently from 75439ed to b712db3 Compare January 16, 2019 11:10
@simonferquel
Copy link
Contributor Author

@silvin-lubecki @vdemeester PTAL :)

assert.Equal(t, c.OwnerReferences[0].APIVersion, v1beta1.SchemeGroupVersion.String())
s, err := secrets.Get("test", metav1.GetOptions{})
assert.NilError(t, err)
assert.Equal(t, len(s.OwnerReferences), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can factorize the check on OwnerReferences with the next test.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

+12k for the vendoring... only for fakes... 😱
Are you sure it's mandatory?

@simonferquel
Copy link
Contributor Author

Fakes are a VERY useful way to test integration with the Kubernetes API, which let us use the existing client-go abstractions without having to write our own fakes. There is a tradeoff there: vendoring the whole thing makes test setup far easier (and cleaner). We could also partially vendor fakes for supporting only the Core API (configs and secrets are in the Core API), but that would require some code change and a more complex test setup. We could also don't vendor the fakes and write our own (but I strongly discourage it).

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@simonferquel simonferquel force-pushed the stack-children-atomic branch from b712db3 to e16a875 Compare January 22, 2019 10:25
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

@silvin-lubecki silvin-lubecki merged commit ebb121e into docker:master Jan 24, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Jan 24, 2019
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.

5 participants