-
Notifications
You must be signed in to change notification settings - Fork 6
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(cli): add @coveo/search-token-server package #33
Conversation
Pull Request Report PR Title ✅ Title follows the conventional commit spec. |
@@ -0,0 +1,8 @@ | |||
# Coveo related configuration | |||
COVEO_PLATFORM_HOSTNAME=<PLATFORM_HOSTNAME dev,qa,prod,hippa> | |||
# for example https://platform.cloud.coveo.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just me, but I would put the comment above the line it explains ;)
And a comment that says to look at README.
In general, don't be afraid to put quite a bit of explanation about what everything is with a lot of detail.
Think from the point of view from a total newbie that just started to play with Coveo interacting through the CLI.
They need explanation about what are the valid platform endpoint, what is an API key/where to see them in your organization, the privileges required, what is a search hub etc..
All of these are well explained in our documentation, so try to always link to an article that would help a total newbie to roughly understand what's going on.
Without that, it is all very cryptic. This module/package can be seen as a good way to teach.
COVEO_PLATFORM_HOSTNAME=<PLATFORM_HOSTNAME dev,qa,prod,hippa> | ||
# for example https://platform.cloud.coveo.com | ||
COVEO_API_KEY=<YOUR_COVEO_API_KEY_HERE> | ||
USER_EMAIL=<YOUR_EMAIL> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
The name of the security identity to impersonate.
Example: "[email protected]"
See https://docs.coveo.com/en/56/#name-string-required.
(If I understand correctly)
Co-authored-by: jpmarceau <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall!
provider: 'Email Security Provider', | ||
})); | ||
|
||
getSearchToken({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use an async await to 'flatten' the code personnaly
*/ | ||
userIds: [ | ||
{ | ||
// The security identities to impersonate when authenticating a query with this search token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this line document the following three lines or just the name? If it's just the name I'd go with:
// The name of the security identity to impersonate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oups. This line should be removed. It's a duplicate of the above line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, GG!
Co-authored-by: jpmarceau <[email protected]>
https://coveord.atlassian.net/browse/CDX-85