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

r/deployment - init container in deployment recreates #977

Closed
wants to merge 3 commits into from

Conversation

DrFaust92
Copy link
Contributor

Description

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch? (If so, please include the test log in a gist)
--- PASS: TestAccKubernetesDeployment_initContainer (74.29s)

Kind of cheated here instead of properly refactoring.

References

Closes #951

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@ghost ghost added size/S labels Sep 1, 2020
@DrFaust92
Copy link
Contributor Author

@jrhouston, open to a better check to see that its not recreated. i just looked at the logs

@jrhouston
Copy link
Contributor

I kind of want to rework this function to have the logic be a bit easier to understand, perhaps an isTemplate variable so we know that the schema is embedded within the template of a deployment, statefulset, or daemonset.

For the moment I think you can actually just remove the line with ForceNew in it because there is a loop at the bottom of the function that will set everything to ForceNew if isUpdatable is set to false:

if !isUpdatable {
for k := range s {
if k == "active_deadline_seconds" {
// This field is always updatable
continue
}
if k == "container" {
// Some fields are always updatable
continue
}
s[k].ForceNew = true
}

@DrFaust92
Copy link
Contributor Author

DrFaust92 commented Sep 24, 2020

@jrhouston, add init_container to the !isUpdatable block as you proposed.
Having issues with local K8s to test this though

@yzargari
Copy link

yzargari commented Nov 9, 2020

This is also happening in daemonset resources. This PR doesn't happen to take care of that by any chance, does it?

@dak1n1
Copy link
Contributor

dak1n1 commented Nov 12, 2020

@yzargari thanks for letting us know! That's really useful information. I'll start working on a new PR that will fix them both, just so that we can move forward quickly with this. @DrFaust92 I'll make sure you get credit too, since there is valuable work in this PR. I just want to do a bunch of testing today to get this change in and it's faster than coordinating.

@DrFaust92
Copy link
Contributor Author

Closing in favor of #1074 @dak1n1 let me know if i can help.

@DrFaust92 DrFaust92 closed this Nov 27, 2020
@ghost
Copy link

ghost commented Dec 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 28, 2020
@DrFaust92 DrFaust92 deleted the r/deployment_init branch April 15, 2021 12:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adding init_container to a deployment causes unnecessary replacement
5 participants