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 Node.js multitenancy guide #2704

Merged
merged 25 commits into from
Mar 13, 2024
Merged

Conversation

Strift
Copy link
Contributor

@Strift Strift commented Feb 1, 2024

Pull Request

Related issue

Fixes #2703

What does this PR do?

This PR repurposes the instructions from this Multi-tenancy guide to create a guide focusing on implementing multi-tenancy with Nodejs.

To be completely honest, I'm a bit hesitant about the value this adds compared to the tenant tokens docs, so feedback is welcome @guimachiavelli @CaroFG

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@meili-bot
Copy link
Collaborator

How to see the preview of this PR?

⚠️ Private link, only accessible to Meilisearch employees.

Go to this URL: https://website-git-deploy-preview-mei-16-meili.vercel.app/docs/branch:add-nodejs-multitenancy-guide

Credentials to access the page are in the company's password manager as "Docs deploy preview".

@CaroFG
Copy link
Contributor

CaroFG commented Feb 7, 2024

Honestly, I don't think this guide adds anything to the documentation. What we explain here is basically the same as what's in the dedicated section of the documentation. I think it could be a bit longer and add something different, like explaining how to connect the back end and front end.

@Strift
Copy link
Contributor Author

Strift commented Feb 7, 2024

Thanks for the feedback @CaroFG, I'll see how I can revisit this.

@Strift Strift marked this pull request as draft February 7, 2024 13:47
@Strift Strift mentioned this pull request Feb 20, 2024
10 tasks
@Strift Strift requested a review from CaroFG February 23, 2024 13:32
@Strift Strift marked this pull request as ready for review February 23, 2024 13:34
@Strift
Copy link
Contributor Author

Strift commented Feb 26, 2024

I've updated the guide. I think this is ready for review @CaroFG @guimachiavelli.

CaroFG
CaroFG previously requested changes Feb 26, 2024
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
Co-authored-by: CaroFG <48251481+CaroFG@users.noreply.github.com>
Copy link
Member

@guimachiavelli guimachiavelli left a comment

Choose a reason for hiding this comment

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

Just a few minor things, mostly suggestions to cut things that might not be necessary.

learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Show resolved Hide resolved
learn/cookbooks/multitenancy_nodejs.mdx Outdated Show resolved Hide resolved
@Strift
Copy link
Contributor Author

Strift commented Mar 12, 2024

Thanks for the review @guimachiavelli 🙏 I applied most suggestions and fixed conflicts. All green to merge on my end 👌

@guimachiavelli
Copy link
Member

Looks good to go for me. I see @CaroFG had a few comments, so I'll wait for her ok before merging

@CaroFG
Copy link
Contributor

CaroFG commented Mar 13, 2024

LGTM!

@guimachiavelli guimachiavelli dismissed CaroFG’s stale review March 13, 2024 15:56

reviewer gave final ok after changes

@guimachiavelli guimachiavelli merged commit fd24bad into main Mar 13, 2024
1 check passed
@guimachiavelli guimachiavelli deleted the add-nodejs-multitenancy-guide branch March 13, 2024 15:57
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.

Add guide on tenant tokens with Node.js
7 participants