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

feat: add meilisearch #2823

Closed
wants to merge 14 commits into from
Closed

feat: add meilisearch #2823

wants to merge 14 commits into from

Conversation

mashail
Copy link
Contributor

@mashail mashail commented Oct 13, 2024

What does this PR do?

Add Meilisearch Golang module

Why is it important?

Meilisearch is getting more popular and there Is already a java module for it.

Related issues

@mashail mashail requested a review from a team as a code owner October 13, 2024 14:35
Copy link

netlify bot commented Oct 13, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 0dd0a1a
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6712b7add39a84000837e8af
😎 Deploy Preview https://deploy-preview-2823--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 26 to 29
// Deprecated: use Run instead
// RunContainer creates an instance of the Meilisearch container type
func RunContainer(ctx context.Context, opts ...testcontainers.ContainerCustomizer) (*MeilisearchContainer, error) {
return Run(ctx, "getmeili/meilisearch:v1.10.3", opts...)
}
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this block of code, I guess you copy-pasted some other module. Could you run the code generation tool for creating modules, as explained here?

It informs of the different alternatives to host the module (here in the core or in your own GH user as a community module), also creating the docs pages for the new module. Finally, it will update the files to include the new module in the CI, among others.

I'd suggest you run the tool first, checking that your code does not include Deprecated code (the tool does not include it)

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelapenya yes true I copied paste from OpenSearch 😄

Ok sure I will run and re-request your review. Thank you @mdelapenya for the comment.

Copy link
Member

Choose a reason for hiding this comment

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

I have a commit with the scaffolding, but I cannot push it because this branch is your main branch. I'll try to submit a PR to your branch so you can incorporate it into this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelapenya
Copy link
Member

@mashail not sure if you check out my previous comment in #2823 (comment), but I can help you out adding the needed changes on top of yours. Wdyt?

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

I left a few suggestions. The most important one is regarding adding the scaffolding (I have a commit for it, I can submit a PR to your branch)

modules/meilisearch/client.http Outdated Show resolved Hide resolved
modules/meilisearch/go.mod Show resolved Hide resolved
modules/meilisearch/go.mod Outdated Show resolved Hide resolved
Comment on lines 26 to 29
// Deprecated: use Run instead
// RunContainer creates an instance of the Meilisearch container type
func RunContainer(ctx context.Context, opts ...testcontainers.ContainerCustomizer) (*MeilisearchContainer, error) {
return Run(ctx, "getmeili/meilisearch:v1.10.3", opts...)
}
Copy link
Member

Choose a reason for hiding this comment

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

I have a commit with the scaffolding, but I cannot push it because this branch is your main branch. I'll try to submit a PR to your branch so you can incorporate it into this one

modules/meilisearch/meilisearch.go Outdated Show resolved Hide resolved
modules/meilisearch/meilisearch.go Outdated Show resolved Hide resolved
modules/meilisearch/meilisearch.go Outdated Show resolved Hide resolved
modules/meilisearch/meilisearch.go Outdated Show resolved Hide resolved
modules/meilisearch/meilisearch.go Outdated Show resolved Hide resolved
modules/meilisearch/meilisearch_test.go Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Oct 17, 2024
@mdelapenya mdelapenya added the enhancement New feature or request label Oct 17, 2024
@mashail
Copy link
Contributor Author

mashail commented Oct 17, 2024

I left a few suggestions. The most important one is regarding adding the scaffolding (I have a commit for it, I can submit a PR to your branch)

@mdelapenya I invited you as a contributor

@mashail mashail requested a review from mdelapenya October 17, 2024 16:36
mashail and others added 2 commits October 17, 2024 19:49
@mashail
Copy link
Contributor Author

mashail commented Oct 17, 2024

@mdelapenya I added the options to the docs

@mashail
Copy link
Contributor Author

mashail commented Oct 18, 2024

@mdelapenya I rebased the branch if you may have a look

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looks like this is missing quite a few fixes from #2835

Which one are we going ahead with, to avoid duplicate effort?

@mdelapenya
Copy link
Member

mdelapenya commented Oct 19, 2024

#2835 is the right one, containing the initial commits from this one to preserve the authors.

In fact, it will close this one when merged.

@stevenh
Copy link
Collaborator

stevenh commented Oct 19, 2024

Thanks I’m going to close this one manually to avoid any confusion

@stevenh stevenh closed this Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants