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 indirection and use a watcher #7

Draft
wants to merge 1 commit into
base: phase1
Choose a base branch
from

Conversation

mateiidavid
Copy link
Collaborator

Lots of indirection and small functions that are used only once. Let's get a brute force solution out before we refactor into smaller pieces. It's still unclear to me how this all API should work, and how it all should tie in.

Instead of using a controller (that reconciles all of the objects) we need to use a reflector (a cache + a watcher). We don't really need to reconcile objects in the same way a deployment needs to be reconciled, I don't think the use of a controller is appropriate.

I would get rid of all of the additional logic that you have and focus on getting the simplest thing out of the door. Do we have an integration test? Do we have a scenario to follow? Logic so far is a bit of a mishmash of ideas and it's hard to follow what we need to do.

I left some todos scattered around (focus on them as you will; I wouldn't focus on the CLI args for example).

We're approaching a point of no return here with choosing our implementation language of choice. I'm still not sure Rust is a good idea given its complexity in initialising watchers. The memory model, borrowing and pinning might be a lot for something that we need to get up-n-running in 1 week from today.

Signed-off-by: Matei David <matei@buoyant.io>
@rushi47
Copy link
Owner

rushi47 commented May 16, 2023

Thanks for the feedback matei, as we discussed on call. Will keep this PR parked for now. And switching implementation to golang.

@rushi47 rushi47 marked this pull request as draft May 16, 2023 23:26
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.

2 participants