Skip to content

Conversation

@javirln
Copy link
Member

@javirln javirln commented Nov 12, 2024

This patch introduces two new columns in the workflows table: latest_run and updated_at. These columns are designed to track the most recent run of a workflow and the last update time, respectively.

Changes in Logic:

  • When a new workflow run is created, the corresponding workflow is updated to reference this run and the updated_at field is set accordingly.
  • The entWrToBizWr function in the data package has been refactored to leverage direct edge relationships, eliminating the need for additional queries to fetch the latest run for specified workflows.

Database Modifications:

  • Addition of latest_run and updated_at columns in the workflows table.
  • A query to update existing workflows so that they reference their most recent workflow run, if available.

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln requested review from jiparis and migmartri November 12, 2024 09:55
@javirln javirln self-assigned this Nov 12, 2024
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Thanks!

field.Bool("public").Default(false),
field.UUID("organization_id", uuid.UUID{}),
field.UUID("project_id", uuid.UUID{}),
field.UUID("latest_run", uuid.UUID{}).Optional().Nillable(),
Copy link
Member

Choose a reason for hiding this comment

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

what does nillable mean compared to optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Optional: Indicates that the field is not required when creating or updating an entity. The field can be omitted in the input data.
  • Nillable: Indicates that the field can hold a nil (or null in SQL) value in the database.

}

// Not efficient, we need to do a query limit = 1 grouped by workflowID
lastRun, err := getLastRun(ctx, workflow)
Copy link
Member

Choose a reason for hiding this comment

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

nice :)


if r != nil {
lastRun, err := entWrToBizWr(ctx, r)
if latestRun := w.Edges.LatestWorkflowRun; latestRun != nil {
Copy link
Member

Choose a reason for hiding this comment

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

should we query if it's not here? We do that pattern in some other places, either take it from the preloaded edge or load it?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I removed the workflowRun option from entWFToBizWF, I added WithLatestWorkflowRun() to all the places where we were previously fetching the latest workflow run on demand. This ensures it works the same as before. My concern with always fetching the latest workflow run when not found on the edge is that it could lead to unnecessary queries since the logic doesn't always use it.

I prefer to keep it this way, and we can update the query if we need the latest workflow run in the future.

return nil, err
}

// Update the workflow with the last run reference
Copy link
Member

Choose a reason for hiding this comment

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

I like that's in the transaction :)

@javirln javirln merged commit e13ca1e into chainloop-dev:main Nov 13, 2024
13 checks passed
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