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

Add feature to allow dry-run for change plans #89

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

rsafonseca
Copy link
Collaborator

No description provided.

main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@L30Bola
Copy link
Member

L30Bola commented Sep 11, 2024

I think that today our whole login is inside of the Reconcile method and this MR shows that. If we want to use the same logic "elsewhere" (in this case it is not elsewhere as we are implementing inside the Reconcile method but I think it should be elsewhere) we would need to copy/paste a lot, if not everything. We should add a TODO on our tasks to actually split the logic from Reconcile into a package (and maybe even more than one package) and Reconcile would only use the package methods and the dry-run itself should be elsewhere (maybe even another codebase/program).

This won't be easy, as I was discussing with @rafatio and he said: "it's not easy to join some of the logic inside a single package". So, yeah, I think that today it's okay the MR but we should do this split in the future.

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.

3 participants