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

ms2/engine unified endpoints #434

Merged
merged 71 commits into from
Jan 28, 2025
Merged

ms2/engine unified endpoints #434

merged 71 commits into from
Jan 28, 2025

Conversation

FelipeTrost
Copy link
Contributor

Summary

  • New type for engines Engine which includes mqtt/http and space/PROCEED engines.
  • Unified engine request interface that takes Engine and makes the appropriate request.

There are still TODOS in the code, but the basic structure is there.

@FelipeTrost FelipeTrost force-pushed the ms2/engine-unified-endpoints branch from 0d64c4c to 09229fe Compare December 10, 2024 16:30
@FelipeTrost FelipeTrost marked this pull request as ready for review December 10, 2024 16:31

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.

Even with enableUseDB is true I get the following error when I navigate to the Instances page for the first time. The page works after refreshing it.
image

The backend shows this error when navigating to the page.
image

Deploying processes does not seem to work either.

Copy link

CLOUDRUN ACTIONS

✅ Successfully created Preview Deployment.

https://pr-434---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.

Looks and works great. There is a small issue that does not affect the functionality too much but should be fixed sometime in the future.

function getLatestVersion(process: DeployedProcessInfo) {
let latest = process.versions.length - 1;
for (let i = process.versions.length - 2; i >= 0; i--) {
if (process.versions[i].version > process.versions[latest].version) latest = i;
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work with the new version info that contains versionId instead of version which is also not usable to infer which version is the newest.

for (const deployment of deployments) {
let latestVesrionIdx = deployment.versions.length - 1;
for (let i = deployment.versions.length - 2; i >= 0; i--) {
if (deployment.versions[i].version > deployment.versions[latestVesrionIdx].version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same problem as in the instance view. I think this will not work correctly since there should not be a version entry in the objects inside versions anymore.

const ExecutionsPage = async ({ params }: { params: { environmentId: string } }) => {
function getDeploymentNames(deployments: DeployedProcessInfo[]) {
for (const deployment of deployments) {
let latestVesrionIdx = deployment.versions.length - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo

@@ -202,7 +224,6 @@ export type VersionInfo = {
versionName: 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 think you need to replace the version entry above with versionId


// Endpoints
type EndpointSchema = typeof import('./endpoints.json');
type Endpoints = EndpointSchema;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed because of the json import or could you just do type Endpoints = typeof import('./endpoints.json'); without the EndpointSchema assignment?

@FelipeTrost FelipeTrost merged commit 34e5592 into main Jan 28, 2025
15 checks passed
@FelipeTrost FelipeTrost deleted the ms2/engine-unified-endpoints branch January 28, 2025 10:15
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.

3 participants