-
-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/mqtt json schema #732
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
base: dev
Are you sure you want to change the base?
Conversation
| }; | ||
|
|
||
| void loadIntegrations(); | ||
| }, []); |
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 think it would be cleaner if components don't do data fetching by themselves.
Instead it should be done in the route loader and passed as props to the component, what do you think?
That will also allow to use the loading (and eventually Suspense) mechanisms.
| .min(1, "Please select at least one sensor"), | ||
| }); | ||
|
|
||
| const mqttSchema = z |
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.
The mqttSchema contains the ttnConfig?
Can we somehow make this dependent on the available integrations? 🤔 Some sort of generic schema or something..
| // .superRefine((data, ctx) => { | ||
| // if (data.mqttEnabled) { | ||
| // // Check required fields when enabled is true | ||
| // if (!data.url) { | ||
| // ctx.addIssue({ | ||
| // path: ["url"], | ||
| // message: "URL is required when MQTT is enabled.", | ||
| // code: "custom", | ||
| // }); | ||
| // } | ||
| // if (!data.topic) { | ||
| // ctx.addIssue({ | ||
| // path: ["topic"], | ||
| // message: "Topic is required when MQTT is enabled.", | ||
| // code: "custom", | ||
| // }); | ||
| // } | ||
| // if (!data.messageFormat) { | ||
| // ctx.addIssue({ | ||
| // path: ["messageFormat"], | ||
| // message: "Message format is required when MQTT is enabled.", | ||
| // code: "custom", | ||
| // }); | ||
| // } | ||
| // } | ||
| // }); |
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.
Remove?
| }); | ||
|
|
||
| const advancedSchema = z.intersection(mqttSchema, ttnSchema) | ||
| // const advancedSchema = z.intersection(mqttSchema, ttnSchema); |
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.
remove?
| [stepper.current.id]: data, | ||
| } | ||
| const onSubmit = (data: FormData) => { | ||
| console.log("🚀 ~ onSubmit ~ data", data); |
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.
lets not keep this :-)
| name: text('name').notNull(), | ||
| slug: text('slug').notNull().unique(), | ||
| serviceUrl: text('service_url').notNull(), | ||
| serviceKeyEnvVar: text('service_key_env_var').notNull(), |
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.
do we care if the serviceKey is an envVar? It is not used as one by osem right?
So why not keep it simple and call it serviceKey?
| slug: text('slug').notNull().unique(), | ||
| serviceUrl: text('service_url').notNull(), | ||
| serviceKeyEnvVar: text('service_key_env_var').notNull(), | ||
| schemaPath: text('schema_path').notNull(), |
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.
Isn't it derivable from the serviceUrl?
Your docs state **GET /integrations/schema/{name}**, but I don't fully get the {name} part.
Can a service provide multiple schemas? If so, this can't just be a column and must be a one to many relationship, no?
| MQTT_SERVICE_URL: z.string(), | ||
| MQTT_SERVICE_KEY: z.string() |
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.
remove for the generic approach?
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 this should be part of the docker-compose file too?
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 you mean to check these in?
They are generated files no?
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Type of Change
Implementation
Checklist
devbranchAdditional Information