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.

Sign up for GitHub

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

Jump to bottom

v0.28: API key updates #1738

Merged
merged 29 commits into from
Jul 7, 2022
Merged

v0.28: API key updates #1738

merged 29 commits into from
Jul 7, 2022

Conversation

maryamsulemani97
Copy link
Contributor

closes #1699

@maryamsulemani97 maryamsulemani97 mentioned this pull request Jun 20, 2022
Closed
@maryamsulemani97 maryamsulemani97 marked this pull request as ready for review June 20, 2022 14:07
@maryamsulemani97 maryamsulemani97 added this to the v0.28 milestone Jun 20, 2022
@maryamsulemani97 maryamsulemani97 requested review from dichotommy and removed request for guimachiavelli June 21, 2022 09:45
@maryamsulemani97 maryamsulemani97 mentioned this pull request Jul 4, 2022
Closed
guimachiavelli
guimachiavelli requested changes Jun 23, 2022
View reviewed changes
Copy link
Member

@guimachiavelli guimachiavelli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great job, but left a few minor comments.

Also, please request a review from an SME so we can be sure we haven't got any technical details wrong.

reference/api/error_codes.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
learn/security/master_api_keys.md Outdated Show resolved Hide resolved
learn/security/master_api_keys.md Outdated Show resolved Hide resolved
learn/security/master_api_keys.md Outdated Show resolved Hide resolved
maryamsulemani97 and others added 3 commits June 23, 2022 17:48
@maryamsulemani97 @guimachiavelli
Apply suggestions from code review
025f332 
Co-authored-by: gui machiavelli <gui@meilisearch.com>
@maryamsulemani97
update based on feedback
cc00994
@maryamsulemani97 @guimachiavelli
Update reference/api/error_codes.md
21880fc 
Co-authored-by: gui machiavelli <gui@meilisearch.com>
@guimachiavelli guimachiavelli linked an issue Jun 23, 2022 that may be closed by this pull request
v0.28: API key updates #1699
Closed
gmourier
gmourier requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@gmourier gmourier left a comment

Choose a reason for hiding this comment

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

  • Some suggestions about pieces of information we may want to tell
  • Updated the description field (the addition of name permits to remove the (/ ) in the description, plus I don't know why but the documentation, the spec, and the code were all out-of-sync regarding the value for the default descriptions). Everything is up-to-date now, the source code being the source of truth.
  • apiKeyUid tenant token claim should contains a valid UUID v4

learn/security/master_api_keys.md Outdated Show resolved Hide resolved
learn/security/tenant_tokens.md Outdated Show resolved Hide resolved
learn/security/tenant_tokens.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
@maryamsulemani97 @gmourier
Apply suggestions from code review
5c28fbf 
Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com>
guimachiavelli
guimachiavelli requested changes Jun 29, 2022
View reviewed changes
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.

Almost there, just a few minor comments.

learn/security/master_api_keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
maryamsulemani97 and others added 2 commits June 29, 2022 18:01
@maryamsulemani97 @guimachiavelli
Apply suggestions from code review
c6215ae 
Co-authored-by: gui machiavelli <gui@meilisearch.com>
@maryamsulemani97
update based on Gui's review
9371bcb
@guimachiavelli guimachiavelli mentioned this pull request Jun 30, 2022
Closed
17 tasks
guimachiavelli
guimachiavelli approved these changes Jun 30, 2022
View reviewed changes
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.

🍎

gmourier
gmourier approved these changes Jul 1, 2022
View reviewed changes
Copy link
Member

@gmourier gmourier left a comment

Choose a reason for hiding this comment

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

🦖

ManyTheFish
ManyTheFish requested changes Jul 4, 2022
View reviewed changes
reference/api/keys.md Outdated Show resolved Hide resolved
reference/api/keys.md Outdated Show resolved Hide resolved
@maryamsulemani97 @ManyTheFish
Update reference/api/keys.md
1727ede 
Co-authored-by: Many the fish <many@meilisearch.com>
gmourier
gmourier requested changes Jul 6, 2022
View reviewed changes
Copy link
Member

@gmourier gmourier left a comment

Choose a reason for hiding this comment

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

One suggestion, make the bash command clearer with a small sentence explaining why it can be useful.

Other than that, I'm fine with anything!

learn/security/master_api_keys.md Show resolved Hide resolved
@maryamsulemani97 maryamsulemani97 requested a review from gmourier July 6, 2022 09:53
gmourier
gmourier requested changes Jul 6, 2022
View reviewed changes
learn/security/master_api_keys.md Outdated Show resolved Hide resolved
@bidoubiwa bidoubiwa mentioned this pull request Jul 6, 2022
Merged
6 tasks
@bidoubiwa bidoubiwa mentioned this pull request Jul 6, 2022
Merged
@guimachiavelli
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Jul 7, 2022

Build succeeded:

@bors bors bot merged commit 700e37d into v0.28 Jul 7, 2022
@bors bors bot deleted the v0.28--API-key-updates branch July 7, 2022 10:21
@bidoubiwa bidoubiwa mentioned this pull request Aug 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bidoubiwa bidoubiwa bidoubiwa left review comments

@curquiza curquiza curquiza left review comments

@Kerollmops Kerollmops Kerollmops left review comments

@guimachiavelli guimachiavelli guimachiavelli approved these changes

@gmourier gmourier gmourier requested changes

@ManyTheFish ManyTheFish ManyTheFish approved these changes

@dichotommy dichotommy Awaiting requested review from dichotommy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.28
Development

Successfully merging this pull request may close these issues.

v0.28: API key updates
7 participants
@maryamsulemani97 @guimachiavelli @Kerollmops @gmourier @ManyTheFish @curquiza @bidoubiwa