Skip to content

Conversation

@javirln
Copy link
Member

@javirln javirln commented Nov 13, 2024

This pull request introduces offset-based pagination to the control plane API, alongside other enhancements. The most important changes include the addition of new pagination request and response types, updates to the workflow service to support filtering and pagination.

Pagination Enhancements:

  • Added OffsetPaginationRequest and OffsetPaginationResponse types to handle offset-based pagination.
  • Updated pagination.proto to include definitions for OffsetPaginationRequest and OffsetPaginationResponse.

Workflow Service Enhancements:

  • Enhanced WorkflowServiceListRequest to support additional filtering options and included OffsetPaginationRequest for pagination.
  • Added OffsetPaginationResponse to WorkflowServiceListResponse to return pagination information.
  • Introduced WorkflowActivityWindow enum to represent the time window for the last known workflow activity.

Possible filters:

  • workflow_name
  • workflow_team
  • project_names
  • visibility of a project
  • workflow run runner type
  • last known workflow run status
  • last known activity of an associated workflow run

Other changes

Removes the parameter project_reference from WorkflowServiceListRequest since it was not used anywhere.

Default behavior

This changes ensures backwards compatibility by making the pagination options, optional and if not provided only returning a specific number of workflows instead. It's advised to update to most recent version of Chainloop once released to leverage the pagination feature.

Ref: #1503

@javirln javirln force-pushed the feat/pfm-1665-logic branch 3 times, most recently from 8d8b3a2 to c2b39db Compare November 13, 2024 12:18
@javirln javirln requested a review from migmartri November 13, 2024 12:18
@javirln javirln self-assigned this Nov 13, 2024
@javirln javirln requested a review from jiparis November 13, 2024 12:18
@javirln javirln changed the title WIP feat(worfklows): Allow server side pagination and filtering for workflows feat(worfklows): Allow server side pagination and filtering for workflows Nov 13, 2024
@javirln javirln marked this pull request as ready for review November 13, 2024 12:37
@javirln javirln marked this pull request as draft November 13, 2024 15:40
@javirln javirln force-pushed the feat/pfm-1665-logic branch from e116e91 to ffea251 Compare November 13, 2024 17:56
@javirln javirln marked this pull request as ready for review November 13, 2024 17:58
…lows

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln force-pushed the feat/pfm-1665-logic branch from ffea251 to 805079f Compare November 13, 2024 18:09
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.

Great, my main concern is about compatibility support in the API and if we have removed any functionality there i.e filter by projectID

}

// OffsetPaginationResponse is used to return the pagination information
message OffsetPaginationResponse {
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, is this a common response type for this kind of pagination?

last_page could be calculated by total_count / page_size >= page?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depending on where you search there is a different response. It seems the most common pattern is:

  • page: current page
  • page_size: the number of results per page
  • total_count: total number of results

I've additionally added the last_page so clients don't need to that exact calculation that you're doing. Instead, another common element is to return the amount of pages. I will remove the last_page and add a total_pages.

// The (zero-based) offset of the first item returned in the collection.
int32 page = 1 [(buf.validate.field).int32.gte = 1];
// The maximum number of entries to return. If the value exceeds the maximum, then the maximum value will be used.
int32 page_size = 2 [(buf.validate.field).int32.gt = 0, (buf.validate.field).int32.lte = 100];
Copy link
Member

Choose a reason for hiding this comment

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

is it mandatory? why don't we have a default?

Copy link
Member Author

Choose a reason for hiding this comment

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

The message itself is not required, only if you provide it. The only default values are in the biz layer. I can add the default values here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have been searching and I couldn't find a way to specify a default value on proto3, apparently it was an option of proto2. I have found this resource https://stackoverflow.com/questions/33222551/why-are-there-no-custom-default-values-in-proto3

In any case, rest assure there are default values if the pagination is not provided a default pagination object is created:

	// Initialize the pagination options, with default values
	paginationOpts := pagination.NewDefaultOffsetPaginationOpts()

	// Override the pagination options if they are provided
	if req.GetPagination() != nil {
		paginationOpts, err = pagination.NewOffsetPaginationOpts(
			int(req.GetPagination().GetPage()),
			int(req.GetPagination().GetPageSize()),
		)
		if err != nil {
			return nil, handleUseCaseErr(err, s.log)
		}
	}

message WorkflowServiceDeleteResponse {}

message WorkflowServiceListRequest {
optional EntityRef project_reference = 1;
Copy link
Member

Choose a reason for hiding this comment

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

this will break compatibility, are we ok with it?

EntityRef allowed us to also find by projectID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we are breaking compatibility but I have search everywhere and we are not using the field anywhere, so my take is that it's safe to be removed.

Yes, EntityRef allowed us to find by projectID, instead now, you can pass repeated number of project names.

// The team the workflow belongs to
string workflow_team = 2 [(buf.validate.field).ignore_empty = true];
// The project the workflow belongs to
repeated string project_names = 3 [(buf.validate.field).ignore_empty = true];
Copy link
Member

Choose a reason for hiding this comment

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

[(buf.validate.field).ignore_empty = true] not necessary is it?

// The name of the workflow to filter by
string workflow_name = 1 [(buf.validate.field).ignore_empty = true];
// The team the workflow belongs to
string workflow_team = 2 [(buf.validate.field).ignore_empty = true];
Copy link
Member

Choose a reason for hiding this comment

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

useless validation? [(buf.validate.field).ignore_empty = true];

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I will check this one and the next one.

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>
@javirln javirln merged commit 2c8a4ff into chainloop-dev:main Nov 14, 2024
13 checks passed
javirln added a commit to javirln/chainloop that referenced this pull request Nov 14, 2024
@migmartri
Copy link
Member

Thanks @javirln for getting back to my questions. LGTM!

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