-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding the v2/Teams API #6
base: main
Are you sure you want to change the base?
Conversation
Hello, thanks for the work. I am on vacation currently but can check in for a review in the next week. I assume you've already used the new APIs and they work for your own needs? |
Hi @danopia! basically, I was writing a script that's using the Teams and the Users API - however, when I found this package I only found the latter being implemented - so I added the former. While I wrote the script I was able to successfully invoke all of the Teams API calls at least once (+ that one v2/users call that I added myself). Does that answer your question? |
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 have two specific requests on this
// Common API client contract | ||
interface ApiClient { | ||
fetchJson(opts: { | ||
method: "GET" | "POST" | "DELETE"; |
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.
It looks like this is the first DELETE call in this project, and so the new method needs to be also added to client.ts
:
Line 40 in 657811e
method?: 'GET' | 'POST'; |
@@ -10,10 +10,37 @@ export interface ResultPage<T> { | |||
data: Array<T>; | |||
} | |||
|
|||
export interface Included { | |||
export interface TeamsResultPage<T> { |
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.
Did datadog really need to add another pagination structure within v2 😅 LGTM
async createTeam(name: string): Promise<string> { | ||
const words = [...name.toLowerCase().matchAll(/[a-z0-9]+/g)].map((x) => x[0]) | ||
const handle = words.join("-") |
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.
This is slightly magic. Does this string manipulation follow a particular recommendation from datadog?
Continued:
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.
oof. I'm afraid I leaked out some business logic from my script here 😅 refactoring in progress
attributes: { | ||
name: name, | ||
handle: handle | ||
}, |
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 see that there are additional possible attributes for this API, so perhaps we can accept a second optional parameter extraAttributes
that lets the caller override handle
and/or pass more attributes for the team.
attributes: { | |
name: name, | |
handle: handle | |
}, | |
attributes: { | |
name: name, | |
handle: handle, | |
...extraAttributes, | |
}, |
Hi! I've noted that in this repo there's no API for v2/Teams, so I added all the GET calls - and also some of the other ones. Please, be kind: this is my literal first OSS contribution in typescript 😅