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 a local run tool for dev purposes #202

Merged
merged 4 commits into from
Dec 18, 2024
Merged

add a local run tool for dev purposes #202

merged 4 commits into from
Dec 18, 2024

Conversation

hervenicol
Copy link
Contributor

What this PR does / why we need it

This should improve dev experience, so we don't have to wait for the CI build.

Checklist

  • Update changelog in CHANGELOG.md.
  • Follow deployment test procedure in the tests/manual_e2e directory and have a working branch.

@hervenicol hervenicol self-assigned this Dec 16, 2024
@hervenicol hervenicol requested a review from a team as a code owner December 16, 2024 13:35
@TheoBrigitte
Copy link
Member

I love it <3 <3

Copy link
Member

@TheoBrigitte TheoBrigitte left a comment

Choose a reason for hiding this comment

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

I like the idea but running outside the cluster makes internal network not reachable, for instance I can't test my Alertmanager controller because it needs to talk to mimir-alertmanager-headless.mimir.svc

hack/bin/run-local.sh Outdated Show resolved Hide resolved
@TheoBrigitte
Copy link
Member

Would you be fine to try a different approach where we replace the docker image in the deployment in order to run it in the cluster ?

@hervenicol
Copy link
Contributor Author

Would you be fine to try a different approach where we replace the docker image in the deployment in order to run it in the cluster ?

That requires extra steps:

  • build the image
  • tag the image
  • login on the registry
  • push the image to a registry
  • update the image tag in the deployment

I'm not sure these extra steps are worth it (especially authenticating on the registry, and making sure we change the image tag - which then implies doing some cleanup for old images).

If your use case is the alertmanager access, why not doing it with a port forward like grafana access?

@TheoBrigitte
Copy link
Member

TheoBrigitte commented Dec 17, 2024

We could use the $USER name to tag the image which make it easy to clean it up afterward.
But this solution might not work because the app would still be reconciled by chart operator eventually and override the docker image "maybe".

Port forward would not work as I cannot resolve mimir-alertmanager-headless.mimir.svc locally; I had a deeper look into this yesterday and there's no way to reach out to CoreDNS as port forward can't expose UDP protocol. Even then k8s network would not be reachable locally, that just does not work.

@hervenicol
Copy link
Contributor Author

All right, I see we have the same behaviour / issue with grafana. Somehow I just checked that the operator was starting and running, but didn't check its actual behaviour and error messages 🤦

I think we should have these parameters (dependencies URLs/ports) as user-definable parameters.
Not only to solve the "how to run the operator locally" question, but also because it's very "us-and-today-specific" to say things like "grafana will run in the monitoring namespace" and "alertmanager will run in the mimir namespace". These should not be hardcoded.

@hervenicol hervenicol mentioned this pull request Dec 18, 2024
2 tasks
@hervenicol
Copy link
Contributor Author

So, now it's working, and you can add the alertmanager setup when available @TheoBrigitte .

@hervenicol hervenicol enabled auto-merge (squash) December 18, 2024 14:04
@hervenicol hervenicol merged commit d61b8b6 into main Dec 18, 2024
8 checks passed
@hervenicol hervenicol deleted the run-local branch December 18, 2024 14:13
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