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

Replace localstack with minio #491

Merged
merged 16 commits into from
Jan 15, 2025
Merged

Conversation

javiermtorres
Copy link
Contributor

@javiermtorres javiermtorres commented Dec 11, 2024

What's changing

The localstack service is replaced with minio for local deployments.

Closes #487

How to test it

Start lumigator (e.g. using make local-up) and check that minio starts instead of localstack.

All automated tests should work as usual.

NOTE: .env file will need to be recreated based on the .env.example (or see .env.example for examples of environment variables that must be configured). This is because some defaults have changed.

Additional notes for reviewers

N/A

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required
    • No DB update is needed.

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-487-add-minio branch 9 times, most recently from adac8d1 to f9cb466 Compare December 11, 2024 15:49
@javiermtorres
Copy link
Contributor Author

One issue I found is that, if the .env.example is modified, then the integration tests should also be run, just in case the env vars are no longer valid. I can make a follow up PR to amend that.

Also, I didn't notice a clear indication that the .env would be generated from the .env.example and used within the docker-compose. If such indication is indeed present, could anyone please point me to it? @agpituk maybe? Thanks in advance.

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-487-add-minio branch from cf607fc to 0ee0acd Compare December 11, 2024 22:50
@aittalam
Copy link
Member

Also, I didn't notice a clear indication that the .env would be generated from the .env.example and used within the docker-compose. If such indication is indeed present, could anyone please point me to it? @agpituk maybe? Thanks in advance.

Not sure if it is in the documentation, but you can find it in the Makefile:

.env:
    @if [ ! -f .env ]; then cp .env.example .env; echo ".env created from .env.example"; fi

docker-compose.yaml Outdated Show resolved Hide resolved
docker-compose.yaml Outdated Show resolved Hide resolved
Copy link
Member

@aittalam aittalam left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @javiermtorres, I think this is great! I tested this and it runs for me (I had a few issues at first, mostly due to old env vars being present and the .env/.env.example thing going on, but afterwards things just ran fine). Accessing all persisted data via Web UI is very cool too :-)

Can I ask you to please go through our sources and look for 4566 and localstack too? I think there are few other spots we might need to update contextually to this change such as:

  • .env
  • documentation (README.md, installation.md, dev.md, etc)

re: conftest.py, I see we are still using localstack. Do you see the need to move tests to minio too? IMHO it is not something urgent, but testing on the same stack we are running makes a bit more sense (and reduces the amount of dependencies + lumigator's fingerprint)

@javiermtorres
Copy link
Contributor Author

@aittalam thanks for trying out the PR and checking that it works :) I'll address the concerns first thing on Monday.
Re: the dependence on localstack for tests.... It's being addressed in #453 , but unfortunately there's one single test case that is inexplicably failing; I cannot even get meaningful logs from Ray. I know that it somehow has to do with inference, but I can't see how. I'll try rebasing to main, but it should be up to date. If something there rings a bell, please please let me know :D

Once that other PR is in place, we'll remove any dependency on localstack in source and tests.

@aittalam
Copy link
Member

@aittalam thanks for trying out the PR and checking that it works :) I'll address the concerns first thing on Monday. Re: the dependence on localstack for tests.... It's being addressed in #453 , but unfortunately there's one single test case that is inexplicably failing; I cannot even get meaningful logs from Ray. I know that it somehow has to do with inference, but I can't see how. I'll try rebasing to main, but it should be up to date. If something there rings a bell, please please let me know :D

Once that other PR is in place, we'll remove any dependency on localstack in source and tests.

That's great @javiermtorres , many thanks!
I'll check #453 today and see if I can be of any help there.

@javiermtorres javiermtorres force-pushed the javiermtorres/issue-487-add-minio branch 3 times, most recently from 379954d to 4bcf822 Compare January 13, 2025 07:58
@javiermtorres javiermtorres force-pushed the javiermtorres/issue-487-add-minio branch from ef2295b to 02c7ffc Compare January 13, 2025 10:09
@javiermtorres
Copy link
Contributor Author

A follow up question: is there any way to persist the minio configuration so we can pick it up if available and run the following only if that is not already set?

Hm. Good question. If the commands are idempotent it doesn't matter (so buckets are not recreated anew if they already exist, etc etc). Was there something specific that failed in your setup?

Leaving the notes on minio persistence here for a future issue, since the original comment is resolved.

Copy link
Member

@aittalam aittalam left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!
I saw the branch got out-to-date wrt main, I pre-approve this so you'll be able to land this after merging from main.

@javiermtorres
Copy link
Contributor Author

Thanks a lot @aittalam :)

@javiermtorres javiermtorres merged commit dca29c3 into main Jan 15, 2025
12 checks passed
@javiermtorres javiermtorres deleted the javiermtorres/issue-487-add-minio branch January 15, 2025 15:58
davidmanzanoai pushed a commit that referenced this pull request Jan 16, 2025
Replace localstack with minio

The S3 support is now provided by minio instead
of localstack. The .env.example has been changed
accordingly. All functionality provided by localstack
should still be available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support minio as an S3-compatible alternative to localstack
4 participants