Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

batches: add ability to filter count and list of BatchSpecs by empty specs#42914

Merged
adeola-ak merged 16 commits into
mainfrom
aakr/exclude-empty-specs
Oct 18, 2022
Merged

batches: add ability to filter count and list of BatchSpecs by empty specs#42914
adeola-ak merged 16 commits into
mainfrom
aakr/exclude-empty-specs

Conversation

@adeola-ak

@adeola-ak adeola-ak commented Oct 13, 2022

Copy link
Copy Markdown
Contributor

closes: #38610

Add filter by empty batch spec option (ExcludeEmptySpecs) to batchSpec schema and update BatchSpecPage query in order to prevent empty batch specs from appearing in the list of executions of a batch change as well as the list of executions on the admin page. Decided to filter out empty batch specs by querying specs with at least 1 field other than name.

prior to fix, with execution spec list including the initiated empty batch spec
Screen Shot 2022-10-13 at 2 56 56 PM

after fix, with execution spec list excluding the initiated empty batch spec
Screen Shot 2022-10-13 at 2 49 04 PM

Test plan

added tests to confirm filter works as well as visually able to see the specs are no longer present

@cla-bot cla-bot Bot added the cla-signed label Oct 13, 2022
@adeola-ak adeola-ak marked this pull request as ready for review October 13, 2022 17:54
@sourcegraph-bot

sourcegraph-bot commented Oct 13, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 21c49c8...76629e9.

Notify File(s)
@courier-new client/web/src/enterprise/batches/BatchSpecsPage.tsx
client/web/src/enterprise/batches/backend.ts
cmd/frontend/graphqlbackend/batches.go
@eseliger client/web/src/enterprise/batches/BatchSpecsPage.tsx
client/web/src/enterprise/batches/backend.ts
cmd/frontend/graphqlbackend/batches.go
enterprise/internal/batches/store/batch_specs.go
enterprise/internal/batches/store/batch_specs_test.go

@courier-new courier-new left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks awesome, and thanks for adding tests for this!

Comment thread cmd/frontend/graphqlbackend/batches.graphql Outdated
Comment thread cmd/frontend/graphqlbackend/batches.graphql Outdated
Comment thread enterprise/internal/batches/store/batch_specs.go
Comment thread enterprise/internal/batches/store/batch_specs.go
adeola-ak and others added 4 commits October 13, 2022 15:07
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
@courier-new

Copy link
Copy Markdown
Contributor

Do you mind adding an item to the Fixed CHANGELOG for this? 🙂


if opts.ExcludeEmptySpecs {
// An empty batch spec's YAML only contains the name, so we filter to batch specs that have at least one key other than "name"
preds = append(preds, sqlf.Sprintf("(EXISTS (SELECT * FROM jsonb_object_keys(batch_specs.spec) AS t (k) WHERE t.k NOT LIKE 'name'))"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

waow what a query 😬

@adeola-ak adeola-ak merged commit 393c148 into main Oct 18, 2022
@adeola-ak adeola-ak deleted the aakr/exclude-empty-specs branch October 18, 2022 14:17
adeola-ak added a commit that referenced this pull request Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initial, empty batch spec is visible from executions list

6 participants