Skip to content
This repository was archived by the owner on Jun 13, 2023. It is now read-only.

Fix the re-apply system#84

Merged
JulienBalestra merged 1 commit intomasterfrom
JulienBalestra/kubectl-apply
Jul 2, 2018
Merged

Fix the re-apply system#84
JulienBalestra merged 1 commit intomasterfrom
JulienBalestra/kubectl-apply

Conversation

@JulienBalestra
Copy link
Copy Markdown
Collaborator

@JulienBalestra JulienBalestra commented Jul 2, 2018

What does this PR do?

After #73, I introduced a regression when using pupernetes reset $ns --apply or SIGSTP.

The re-apply feature doesn't work anymore.

To avoid this kind of regression I added a test in the CI.

Additional Notes

This should be short released on v0.6.1.

@JulienBalestra JulienBalestra added the bug Something isn't working label Jul 2, 2018
@JulienBalestra JulienBalestra added this to the v0.6.1 milestone Jul 2, 2018
@JulienBalestra JulienBalestra requested review from a team and kindermoumoute July 2, 2018 17:32
Comment thread pkg/run/run.go
glog.Errorf("Cannot apply manifests in %s", r.env.GetManifestsPathToApply())
continue
}
r.state.SetKubectlApplied()
Copy link
Copy Markdown
Contributor

@CharlyF CharlyF Jul 2, 2018

Choose a reason for hiding this comment

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

Why don't we run a r.UnsetKubectlApplied()

// UnsetKubectlApplied mark the state when kubectl reset successfully returned
func (s *State) UnsetKubectlApplied() {
	s.Lock()
	s.kubectlApplied = false
	s.Unlock()
}

around: https://github.com/DataDog/pupernetes/blob/master/pkg/run/delete.go#L322 ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We would need more code about the namespace no?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, for sure - I just want to make sure resetting allows for a fresh start and thus one could reset the state 2, 3, N times in a row with no issues.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The reset capability is managed by a dedicated channel, so I think it's safe and simple to let it like that and this restore the previous logic.
Then, it's not that bad if we apply when not needed because the kubectl apply is idempotent, this is safe.
Today we only have some resources in the kube-system namespace, but we could have something else in an another namespace. It let to the user a way to configure it as it suits him.

Copy link
Copy Markdown
Contributor

@CharlyF CharlyF left a comment

Choose a reason for hiding this comment

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

🚀

@JulienBalestra JulienBalestra merged commit 56d511c into master Jul 2, 2018
@JulienBalestra JulienBalestra deleted the JulienBalestra/kubectl-apply branch July 2, 2018 21:38
@JulienBalestra JulienBalestra mentioned this pull request Jul 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants