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

[DIR-1444] [DIR-724] [DIR-1506] migrate to new instances api #1497

Merged

Conversation

stefan-kracht
Copy link
Member

@stefan-kracht stefan-kracht commented Apr 25, 2024

Description

Migrating to the v2 instance API

  • The instance list endpoint does not support streaming anymore, I added a reload button to the top of the list
  • instances can now also be filtered by status === crashed (the possible statuses were hard-coded in the filter component, I added a direct binding to the schema so that every new status will automatically be added to the filter)
  • the streaming on the details page will now stop when the instance is not running anymore (this saves some browser resources, the streaming endpoint pushes updates very frequently, even when nothing changes)
  • found a way to fix DIR-724
  • I updated the API commands button on the workflow page to use the new API (this closes DIR-1506)
  • I removed the api/tree folder after we migrated all APIs (the FileNameSchema is the only thing we still use, I moved that into src/api/files/schema.ts)
  • The new API design fixes a major bug in the diagram on the instance page. The diagram was always generated based on the current file content of the workflow that triggered the instance. This workflow might have changed since the workflow ran, and the diagram could show something completely wrong. If the file was deleted or renamed after the instance was generated, the diagram would not have been displayed at. With the new API, the content of the workflow that created the instance will be part of the response and can be used as a reliable source for the diagram.

Follow-up tickets

To let us move a bit quicker in the API migration, I added the following task as a separate ticket as I think it is a good iteration and not a mandatory

Checklist

  • Documentation updated if required
  • Test coverage is appropriate

Checklist Internal

  • Linear issue linked (e.g. [DIR-XXXX] pull request title)
  • Has the PR been labeled

Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
import { z } from "zod";

export const WorkflowStartedSchema = z.object({
  namespace: z.string(),
  instance: z.string(),
});

export const fileNameSchema = z
  .string()
  .regex(/^(([a-z][a-z0-9_\-.]*[a-z0-9])|([a-z]))$/, {
    message:
      "Please use a name that only contains lowercase letters, use - or _ instead of whitespaces.",
  });

Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
@stefan-kracht stefan-kracht self-assigned this Apr 25, 2024
stefan-kracht and others added 18 commits April 25, 2024 15:40
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
@stefan-kracht stefan-kracht requested a review from sebxian May 3, 2024 11:34
Copy link
Contributor

@sebxian sebxian left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Some comments in the code regarding naming.

I also have one question -- this is probably outside of the scope of this PR, but for clarification: Was there a discussion with the backend guys regarding the format of the filter params? I think they could be improved, and that might also lead to a simplification of currently hard coded stuff in the ui. But that would probably be a separate issue.

ui/src/api/instances/index.ts Show resolved Hide resolved
ui/src/api/instances/query/get.ts Outdated Show resolved Hide resolved
ui/src/pages/namespace/Instances/Detail/Header/index.tsx Outdated Show resolved Hide resolved
ui/src/pages/namespace/Monitoring/Instances/index.tsx Outdated Show resolved Hide resolved
ui/src/pages/namespace/Monitoring/Instances/index.tsx Outdated Show resolved Hide resolved
stefan-kracht and others added 6 commits May 6, 2024 13:41
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
Signed-off-by: Sebastian Armbrust <sebastian.armbrust@direktiv.io>
Signed-off-by: Stefan Kracht <stefan.kracht@direktiv.io>
@stefan-kracht
Copy link
Member Author

This looks good to me!

Some comments in the code regarding naming.

I also have one question -- this is probably outside of the scope of this PR, but for clarification: Was there a discussion with the backend guys regarding the format of the filter params? I think they could be improved, and that might also lead to a simplification of currently hard coded stuff in the ui. But that would probably be a separate issue.

No, there was no discussion of changing the query parameters. Maybe we make this is a future ticket and align the params with the event filter ones.

@stefan-kracht stefan-kracht requested a review from sebxian May 6, 2024 12:21
Signed-off-by: Sebastian Armbrust <sebastian.armbrust@direktiv.io>
Signed-off-by: Sebastian Armbrust <sebastian.armbrust@direktiv.io>
Copy link
Contributor

@sebxian sebxian left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. As discussed, I pushed some more changes to get all the spelling consistent. Good idea about a follow-up for event & instances filters, I'll keep this in mind while working on the events!

@stefan-kracht stefan-kracht merged commit 5266065 into main May 6, 2024
18 of 19 checks passed
@stefan-kracht stefan-kracht deleted the stefankracht/dir-1444-migrate-to-new-instances-api branch May 6, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants