Skip to content

[resource-controller] rename createOrUpdateResource ? #374

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

Closed
lburgazzoli opened this issue Mar 15, 2021 · 2 comments · Fixed by #646
Closed

[resource-controller] rename createOrUpdateResource ? #374

lburgazzoli opened this issue Mar 15, 2021 · 2 comments · Fixed by #646
Assignees
Labels
api-changes-epic java Pull requests that update Java code
Milestone

Comments

@lburgazzoli
Copy link
Collaborator

ResourceController's main entry point is createOrUpdateResource(R resource, Context<R> context) seems misleading as in fact the method operates on an already existing CR so not sure if create is actually an option.

Maybe reconcile would be a better name ?

@csviri
Copy link
Collaborator

csviri commented Apr 9, 2021

@lburgazzoli I agree, reaised this actually too. The original idea was that we create or update resources managed by the controller so it's meant like that (not creating or updating the CR). Or in event context handle createOrUpdate even of a custom resource. However we are currently also support different events from differnet events sources, so this naming does not fit at all IMHO. So the correct name should be reconcile.

In addition to that also the delete method should be rather cleanup.

@csviri
Copy link
Collaborator

csviri commented Nov 5, 2021

Created a separate issues where described the current changes, please take a look, we can continue the discussion there:
#655

will close this issues for now.

@csviri csviri closed this as completed Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes-epic java Pull requests that update Java code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants