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

docs: Instance admin #3961

Merged
merged 7 commits into from
Jun 13, 2023
Merged

docs: Instance admin #3961

merged 7 commits into from
Jun 13, 2023

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented Jun 12, 2023

What

Adds the documentation for the instance admin endpoints.

@chriswk chriswk self-assigned this Jun 12, 2023
@vercel
Copy link

vercel bot commented Jun 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 13, 2023 0:09am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Jun 13, 2023 0:09am

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Overall this looks very good! 💯

I have left a few small comments and questions that I think we should address before merging, but it should all be pretty trivial.

src/lib/openapi/spec/instance-admin-stats-schema.ts Outdated Show resolved Hide resolved
schemaName: string,
): OpenAPIV3.ResponseObject => {
return createResponseSchemas(schemaName, {
'text/csv': schemaNamed(schemaName),
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Does this convert it to CSV (for examples etc)? Does the example response show as CSV in swagger UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately swagger does not show examples for CSV

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ... Yeah, it doesn't look like it's well supported. Can we still show examples, though? according to this stack overflow thread, you should mark CSV payloads as strings if I read correctly. And they can probably have examples. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is annoying, the schema still shows the json example, but we definitely have an object here, so I'm inclined to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, which means we should just drop the schemaNamed here then. and just use string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it by making the type of the response string, and then allowing the makeCsvResponseSchema take an example string. Our example is now a csv serialized object of the type we're expecting.

Comment on lines 42 to 59
openApiService.validPath({
tags: ['Instance Admin'],
operationId: 'getInstanceAdminStatsCsv',
responses: {
200: createCsvResponseSchema(
'instanceAdminStatsSchema',
),
},
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a summary and a description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Oversight from my side.

@@ -46,6 +60,10 @@ class InstanceAdminController extends Controller {
openApiService.validPath({
tags: ['Instance Admin'],
operationId: 'getInstanceAdminStats',
summary:
'An overview of usage of various features of Unleash',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can shorten it down a bit?

Suggested change
'An overview of usage of various features of Unleash',
'Get feature usage overview',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we've conflated the word feature here. Maybe "Get usage statistics" instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's much better 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up using Instance usage statistics

src/lib/routes/admin-api/instance-admin.ts Outdated Show resolved Hide resolved
Comment on lines 38 to 88
description: 'The number of users this instance has',
example: 8,
},
featureToggles: {
type: 'number',
description: 'The number of feature-toggles this instance has',
example: 47,
},
projects: {
type: 'number',
description: 'The number of projects defined in this instance.',
example: 3,
},
contextFields: {
type: 'number',
description:
'The number of context fields defined in this instance.',
example: 7,
},
roles: {
type: 'number',
description: 'The number of roles defined in this instance',
example: 5,
},
groups: {
type: 'number',
description: 'The number of groups defined in this instance',
example: 12,
},
environments: {
type: 'number',
description: 'The number of environments defined in this instance',
example: 3,
},
segments: {
type: 'number',
description: 'The number of segments defined in this instance',
example: 19,
},
strategies: {
type: 'number',
description: 'The number of strategies defined in this instance',
example: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

We could mark all these numbers in the admin stats schema with a minimum: 0. That would be more accurate. However, because this is a response schema, we're not in any danger of the users submitting wrong data based on this, so it's not that important.

What do you think? I think it'd be nice, but I'll leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I mean, when parsing this that would allow me to use unsigned types safely. Compared to how little work it is to add, happy to add it.

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Super 👏🏼 This looks great 😄

@chriswk chriswk enabled auto-merge (squash) June 13, 2023 12:02
@chriswk chriswk merged commit 5f3e572 into main Jun 13, 2023
10 checks passed
@chriswk chriswk deleted the docs/instanceAdmin branch June 13, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants