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

Allow filtering builds by event #3103

Closed

Conversation

julienduchesne
Copy link

@julienduchesne julienduchesne commented Jul 13, 2021

A user can now pass &event=cron|push|pull_request|...

The WHERE condition is built like ('' = :build_event OR build_event = :build_event) to allow the same query to be used when an event is specified and when it isn't (empty string)
As such, the List and ListRef methods were also merged into one using the same pattern

This is done to support drone/drone-ui#356. I know that the support of filtering was riding on the implementation so this PR can serve as a discussion base
Let me know what you think of this

The WHERE condition is built like `('' = :build_event OR build_event = :build_event)` to allow the same query to be used when an event is specified and when it isn't (empty string)
As such, the `List` and `ListRef` methods were also merged into one using the same pattern

This is done to support drone/drone-ui#356. I know that the support of filtering was riding on the implementation so this PR can serve as a discussion base
Let me know what you think of this
@CLAassistant
Copy link

CLAassistant commented Jul 13, 2021

CLA assistant check
All committers have signed the CLA.

@julienduchesne julienduchesne marked this pull request as ready for review July 13, 2021 13:00
@bradrydzewski
Copy link

I recommend using a query builder here instead:

var buf strings.Builder
buf.WriteString(queryBase)
buf.Writestring("FROM builds WHERE build_repo_id = :build_repo_id")
if params.Event != "" {
  buf.WriteString("AND build_ref = :build_ref")
}
buf.WriteString(`ORDER BY build_id DESC LIMIT :limit OFFSET :offset`)

And modifying the signature so we have more flexibility in the future to add more filtering criteria:

func (s *buildStore) List(ctx context.Context, repo int64, opts core.BuildListOpts) ([]*core.Build, error)

@marko-gacesa
Copy link
Contributor

Hey @julienduchesne, thanks for your contribution. I'm going to work on the changes that @bradrydzewski suggested. I'm going to build them on top of your commit. When I'm done I'll create a new pull request.

@hitesharinga hitesharinga changed the base branch from master to drone October 4, 2023 02:44
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.

None yet

4 participants