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: Events tag #4152

Merged
merged 4 commits into from
Jul 6, 2023
Merged

docs: Events tag #4152

merged 4 commits into from
Jul 6, 2023

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented Jul 5, 2023

What

This PR adds documentation for our endpoints that are covered by our "Events" tag. It also adds a type for all valid events, and then uses this as valid values for type argument.

@vercel
Copy link

vercel bot commented Jul 5, 2023

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

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Jul 6, 2023 8:47am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Jul 6, 2023 8:47am

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.

As always: very good work on this! I've got a few suggestions going through, but nothing major. Feel free to handle and merge as you please.

Comment on lines 10 to 30
name: {
type: 'string',
},
description: {
type: 'string',
},
type: {
type: 'string',
},
project: {
type: 'string',
},
stale: {
type: 'boolean',
},
variants: {
type: 'array',
items: {
$ref: '#/components/schemas/variantSchema',
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I know these properties are only here to allow the example (or at least that's what I seem to remember), but could we still give them a description? It might be hard because they can vary between events, though 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I can still give it a go certainly, I can steal with pride from our feature schema 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Just try and make it clear in some way that the descriptions may apply to different things depending on what the event is for

Comment on lines +15 to +16
description:
'The api version of this response. A natural increasing number. Only increases if format changes',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest maybe even adding an enum here, so that we have to explicitly mark it when we bump a version. Makes it easier for users to know what to expect. Also: this needs an example

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.

Comment on lines 14 to 16
description: 'An API versioning number',
minimum: 1,
example: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above regarding enums

Comment on lines 50 to 51
adminRole = rootRoles.find((r) => r.name === RoleName.ADMIN)!!;
viewerRole = rootRoles.find((r) => r.name === RoleName.VIEWER)!!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need double bangs or is it enough with a single one? Think I saw this somewhere else and that a single one will suffice 💁🏼

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 think I might've inadvertently used Kotlin syntax here. Let me see what the compiler says with a single bang :P

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, a single bang is enough. Will update.

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

LGTM!

type: 'string',
},
type: {
type: 'string',
Copy link
Contributor

Choose a reason for hiding this comment

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

// I wonder if we can inject the enum type here so we don't have to maintain the list in two places

Suggested change
type: 'string',
type: 'string',
example: 'APPLICATION_CREATED',

Copy link
Contributor

Choose a reason for hiding this comment

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

Good shout! That should be possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, cause it's not the event type, it could be the feature type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example for our feature-created events has type: 'release' here. So unfortunately, we can't inject the event type enum here.

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 have a union type for event-type + feature-type, but I think that'd be requesting too much on this PR. This is good for me 👍


const eventDataSchema = {
type: 'object',
additionalProperties: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have some required properties? Maybe

Suggested change
additionalProperties: true,
additionalProperties: true,
required: ['name', 'type'],

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 don't believe we do. There's nothing preventing you from creating an event of {}, if you just want to use the event to trigger some other behaviour.

@@ -3,115 +3,222 @@ import { FeatureToggle, IStrategyConfig, ITag, IVariant } from './model';
import { IApiToken } from './models/api-token';
import { IUser } from './user';

export const APPLICATION_CREATED = 'application-created';
export const APPLICATION_CREATED = 'application-created' as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

@chriswk chriswk enabled auto-merge (squash) July 6, 2023 08:51
@chriswk chriswk merged commit b04545c into main Jul 6, 2023
10 checks passed
@chriswk chriswk deleted the docs/eventsTag branch July 6, 2023 08:57
export const FEATURES_EXPORTED = 'features-exported' as const;
export const FEATURES_IMPORTED = 'features-imported' as const;

export const IEventTypes = [
Copy link
Contributor

Choose a reason for hiding this comment

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

@chriswk by making it this strict, we can't build unleash-enterprise, as there are types in enterprise which are not defined here: https://github.com/ivarconr/unleash-enterprise/actions/runs/5473805611/jobs/9967754171 I think we should make it string again

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

3 participants