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

Remove code-path for responding to Delete events #101

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gregjones
Copy link
Contributor

Now that we are setting ownerReferences on the created objects
kubernetes will take care of deleting them for us when the application
is deleted, so we can remove all the code related to that.

The delete methods for service, ingress and autoscaler are still needed
for the case where the config changes such that they are no longer
required, but now they can be private/internal methods.

Now that we are setting ownerReferences on the created objects
kubernetes will take care of deleting them for us when the application
is deleted, so we can remove all the code related to that.

The delete methods for service, ingress and autoscaler are still needed
for the case where the config changes such that they are no longer
required, but now they can be private/internal methods.
@gregjones gregjones requested a review from a team as a code owner May 28, 2020 13:03
xavileon
xavileon previously approved these changes May 28, 2020
Copy link
Contributor

@xavileon xavileon left a comment

Choose a reason for hiding this comment

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

🔥

mortenlj
mortenlj previously approved these changes May 28, 2020
Copy link
Member

@oyvindio oyvindio left a comment

Choose a reason for hiding this comment

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

I think this codepath can also be removed:

elif event.action == "DELETE":
self._delete(event.app_spec)

@gregjones gregjones dismissed stale reviews from mortenlj and xavileon via 5b79aa5 May 29, 2020 08:53
@gregjones
Copy link
Contributor Author

@oyvindio Yes, I missed that part. It's deleted with 5b79aa5

Copy link
Member

@oyvindio oyvindio left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gregjones
Copy link
Contributor Author

Thanks for the reviews. As mentioned elsewhere, I'll wait on merging this until we are ready to trigger re-deploys of the apps in our clusters, I think it's relatively harmless being there and doing nothing in the short-term.

@mortenlj
Copy link
Member

Would be nice to just get this merged and out of the way, wouldn't it? 🙂

@mortenlj
Copy link
Member

I think this PR needs to be merged or closed.

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.

4 participants