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

Initial mqtt calls for endpoints #403

Merged
merged 21 commits into from
Nov 28, 2024
Merged

Initial mqtt calls for endpoints #403

merged 21 commits into from
Nov 28, 2024

Conversation

FelipeTrost
Copy link
Contributor

Summary

Initial implementation of mqtt requests for engines and overview of engines for admins.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@jjoderis jjoderis left a comment

Choose a reason for hiding this comment

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

When i go to /admin/engines i get an error.

image

@FelipeTrost
Copy link
Contributor Author

When i go to /admin/engines i get an error.

@jjoderis did you have a mqtt broker running and the env vars set?

@jjoderis
Copy link
Contributor

Ah i see. No i didn't. Would it be a valid option to hide the entry from the Dashboard if the mqtt environment values are not set? From what i understand the route cannot work without them.
And are there upsides and downsides to using environment variables instead of configuration entries for this? I guess it would not really be possible to change the mqtt server address or password while the MS is running using environment variables

@FelipeTrost
Copy link
Contributor Author

I think it would be better to show a message that mqtt isn't set up, what do you think?

I remember Kai saying that he wanted the mqtt credentials in the environment variables, I think for now it is ok.

@jjoderis
Copy link
Contributor

I see. I think a specific message about mqtt not being set up would possibly improve the user experience.

This comment has been minimized.

Copy link

CLOUDRUN ACTIONS

✅ Successfully created Preview Deployment.

https://pr-403---ms-server-staging-c4f6qdpj7q-ew.a.run.app

Copy link
Contributor

@jjoderis jjoderis left a comment

Choose a reason for hiding this comment

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

LGTM
Maybe add a some comments to your typescript endpoint helpers to make it easier to grasp what they do and they work.

type Endpoints = EndpointSchema;
type Methods = 'get' | 'post' | 'put' | 'delete';

type GetParamsFromString<
Copy link
Contributor

@jjoderis jjoderis Nov 28, 2024

Choose a reason for hiding this comment

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

I would maybe add one or two comments to this file to explain what exactly these types do.
And is "Count" actually a fitting name? I looks like it is a list of all arguments that are behind a ":" in the string.

@FelipeTrost FelipeTrost merged commit cb7885c into main Nov 28, 2024
15 checks passed
@FelipeTrost FelipeTrost deleted the engine/mqtt branch November 28, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants