Skip to content

Commit 658427c

Browse files
post Saved Objects CI check feedback as a PR comment (#268469)
resolves elastic/kibana-team#3141 ## Summary Today, when the Saved Objects CI check runs on a PR, the only signal a developer gets is a green or red tile in Buildkite. If it fails, you have to dig through nested build logs (or ping the Core team) to figure out which rule fired and how to fix it. If it passes but you changed a Saved Object type, there's no nudge to think about whether your change needs a [2-step release](https://www.elastic.co/docs/extend/kibana/saved-objects#defining-model-versions). This PR makes the check explain itself directly on the PR. After every run on a pull request, you'll see one of: - **Failure** — a comment listing the violations grouped by SO type, with a one-line fix hint and a link to the relevant docs section. - **Success with SO changes** — a comment summarizing which types were added / updated / removed, with a 2-step-release reminder when applicable. - **Success with no SO changes** — no comment. The comment is idempotent: re-runs replace it instead of stacking new ones, the same pattern already used by the API contracts notifier. This closes the 4th done-criterion of elastic/kibana-team#3139. ### How it works (high level) 1. The CLI (`scripts/check_saved_objects`) now emits a structured JSON report alongside its normal output (new `--report-path` flag). 2. A small Buildkite script reads that report and posts (or updates) the PR comment. 3. Validators have been updated to attach a stable rule ID, fix hint, and docs link to each violation, so the comment can render them as actionable items rather than raw stack traces. ## How to test **Local smoke (no comment posted):** ``` node scripts/check_saved_objects --baseline <merge-base-sha> --report-path /tmp/r.json cat /tmp/r.json | jq ``` **Unit tests:** ``` node scripts/jest packages/kbn-check-saved-objects-cli node scripts/jest .buildkite/scripts/steps/checks/notify_saved_objects_changes.test.ts ``` ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) — N/A, CI-only output (Buildkite logs + GitHub PR markdown). - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. — no HTTP API changes. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks - **Comment spam on re-run** — avoided by re-using a stable comment context, so each new run updates the existing comment in place. - **Notifier failure breaks the build** — the notifier is fire-and-forget; if it errors, the step still passes and prints a warning. - **Soft-fail still on** — `.buildkite/pipelines/pull_request/check_saved_objects.yml` keeps `soft_fail: true` for now. Flipping it to a hard fail is a separate done-criterion of elastic/kibana-team#3139. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 487d488 commit 658427c

17 files changed

Lines changed: 868 additions & 83 deletions

.buildkite/scripts/steps/check_saved_objects.sh

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,24 @@ if is_pr; then
103103
SERVERLESS_BASELINE_FLAG=(--serverless-baseline "$GITHUB_SERVERLESS_BASELINE_SHA")
104104
fi
105105

106+
SO_REPORT_PATH="$(mktemp -t so-check-report.XXXXXX).json"
107+
CHECK_EXIT=0
108+
106109
if ! is_auto_commit_disabled; then
107-
# The step might update files like removed_types.json and/or SO fixtures
108-
node scripts/check_saved_objects --baseline "$MERGE_BASE_REV" "${SERVERLESS_BASELINE_FLAG[@]}" --algorithm both --fix
110+
# The step might update files like removed_types.json and/or SO fixtures.
111+
# `check_for_changed_files` runs unconditionally so that any files produced by --fix
112+
# are auto-committed even when the check also reports non-fixable violations.
113+
node scripts/check_saved_objects --baseline "$MERGE_BASE_REV" "${SERVERLESS_BASELINE_FLAG[@]}" --algorithm both --report-path "$SO_REPORT_PATH" --fix || CHECK_EXIT=$?
109114
check_for_changed_files "node scripts/check_saved_objects" true
110115
else
111-
node scripts/check_saved_objects --baseline "$MERGE_BASE_REV" "${SERVERLESS_BASELINE_FLAG[@]}" --algorithm both
116+
node scripts/check_saved_objects --baseline "$MERGE_BASE_REV" "${SERVERLESS_BASELINE_FLAG[@]}" --algorithm both --report-path "$SO_REPORT_PATH" || CHECK_EXIT=$?
117+
fi
118+
119+
echo --- Post Saved Objects PR comment
120+
ts-node .buildkite/scripts/steps/checks/notify_saved_objects_changes.ts --report-path "$SO_REPORT_PATH" || echo "Warning: failed to post Saved Objects PR notification"
121+
122+
if [[ "$CHECK_EXIT" -ne 0 ]]; then
123+
exit "$CHECK_EXIT"
112124
fi
113125
else
114126
# We are on the 'on-merge' pipeline, the goal is to test against current serverless release,
Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
jest.mock('#pipeline-utils', () => ({
11+
upsertComment: jest.fn(),
12+
}));
13+
14+
import {
15+
buildCommentBody,
16+
buildFailureBody,
17+
buildSuccessBody,
18+
type SavedObjectsCheckFinding,
19+
type SavedObjectsCheckReport,
20+
} from './notify_saved_objects_changes';
21+
22+
const finding = (overrides: Partial<SavedObjectsCheckFinding> = {}): SavedObjectsCheckFinding => ({
23+
ruleId: 'existing-type/mutated-existing-model-version',
24+
severity: 'error',
25+
typeName: 'task',
26+
message: "Some modelVersions have been updated for SO type 'task'.",
27+
fixHint: 'Existing model versions are immutable.',
28+
docsAnchor: '#defining-model-versions',
29+
...overrides,
30+
});
31+
32+
const report = (overrides: Partial<SavedObjectsCheckReport> = {}): SavedObjectsCheckReport => ({
33+
status: 'pass',
34+
baseline: 'abc123',
35+
newTypes: [],
36+
updatedTypes: [],
37+
removedTypes: [],
38+
findings: [],
39+
...overrides,
40+
});
41+
42+
describe('buildCommentBody', () => {
43+
it('returns null on a clean pass with no SO changes', () => {
44+
expect(buildCommentBody(report())).toBeNull();
45+
});
46+
47+
it('returns the success body when the run passed and SO types were touched', () => {
48+
const body = buildCommentBody(report({ updatedTypes: ['task'] }));
49+
expect(body).not.toBeNull();
50+
expect(body).toContain('Saved Objects CI check passed');
51+
});
52+
53+
it('returns the failure body when the run failed', () => {
54+
const body = buildCommentBody(report({ status: 'fail', findings: [finding()] }));
55+
expect(body).toContain('Saved Objects CI check failed');
56+
});
57+
});
58+
59+
describe('buildSuccessBody', () => {
60+
it('lists every change category that has entries', () => {
61+
const body = buildSuccessBody(
62+
report({
63+
newTypes: ['shiny-new-type'],
64+
updatedTypes: ['task', 'config'],
65+
removedTypes: ['old-type'],
66+
})
67+
);
68+
69+
expect(body).toContain('**New types:** `shiny-new-type`');
70+
expect(body).toContain('**Updated types:** `task`, `config`');
71+
expect(body).toContain('**Removed types:** `old-type`');
72+
});
73+
74+
it('includes the 2-step release reminder when types were updated', () => {
75+
const body = buildSuccessBody(report({ updatedTypes: ['task'] }));
76+
expect(body).toMatch(/2-step release/);
77+
expect(body).toContain('https://www.elastic.co/docs/extend/kibana/saved-objects');
78+
});
79+
80+
it('includes the 2-step release reminder when types were removed', () => {
81+
const body = buildSuccessBody(report({ removedTypes: ['old-type'] }));
82+
expect(body).toMatch(/2-step release/);
83+
});
84+
85+
it('omits the 2-step release reminder for pure new-type additions', () => {
86+
const body = buildSuccessBody(report({ newTypes: ['shiny-new-type'] }));
87+
expect(body).not.toMatch(/2-step release/);
88+
expect(body).toContain('**New types:** `shiny-new-type`');
89+
});
90+
});
91+
92+
describe('buildFailureBody', () => {
93+
it('groups findings by typeName and emits per-rule bullets with a docs link', () => {
94+
const body = buildFailureBody(
95+
report({
96+
status: 'fail',
97+
findings: [
98+
finding({ typeName: 'task', ruleId: 'existing-type/mutated-existing-model-version' }),
99+
finding({
100+
typeName: 'task',
101+
ruleId: 'existing-type/mappings-changed-without-new-model-version',
102+
message: 'mappings changed without a new model version',
103+
fixHint: 'add a model version',
104+
}),
105+
finding({
106+
typeName: 'config',
107+
ruleId: 'new-type/legacy-migrations',
108+
message: "New SO type 'config' cannot define legacy migrations",
109+
fixHint: 'remove migrations',
110+
}),
111+
],
112+
})
113+
);
114+
115+
expect(body).toContain('### `task`');
116+
expect(body).toContain('### `config`');
117+
expect(body).toContain('**[existing-type/mutated-existing-model-version]**');
118+
expect(body).toContain('**[new-type/legacy-migrations]**');
119+
expect(body).toContain(
120+
'([docs](https://www.elastic.co/docs/extend/kibana/saved-objects#defining-model-versions))'
121+
);
122+
expect(body).toMatch(/3 issue\(s\) across 2 type\(s\)/);
123+
});
124+
125+
it('files findings without a typeName under a General section', () => {
126+
const body = buildFailureBody(
127+
report({
128+
status: 'fail',
129+
findings: [
130+
finding({ typeName: undefined, ruleId: 'generic', message: 'something blew up' }),
131+
],
132+
})
133+
);
134+
135+
expect(body).toContain('### General');
136+
expect(body).toContain('**[generic]** something blew up');
137+
});
138+
139+
it('embeds a reproducible local command using the baseline SHA', () => {
140+
const body = buildFailureBody(
141+
report({ status: 'fail', baseline: 'deadbeef', findings: [finding()] })
142+
);
143+
144+
expect(body).toContain('node scripts/check_saved_objects --baseline deadbeef');
145+
});
146+
147+
it('falls back to a placeholder when no baseline is available', () => {
148+
const body = buildFailureBody(
149+
report({ status: 'fail', baseline: undefined, findings: [finding()] })
150+
);
151+
152+
expect(body).toContain('node scripts/check_saved_objects --baseline <merge-base-sha>');
153+
});
154+
155+
it('omits the fix hint when not provided', () => {
156+
const body = buildFailureBody(
157+
report({
158+
status: 'fail',
159+
findings: [finding({ fixHint: undefined })],
160+
})
161+
);
162+
163+
expect(body).not.toContain('_Fix:_');
164+
});
165+
166+
describe('when the run failed but no findings were collected', () => {
167+
const originalBuildUrl = process.env.BUILDKITE_BUILD_URL;
168+
afterEach(() => {
169+
if (originalBuildUrl === undefined) {
170+
delete process.env.BUILDKITE_BUILD_URL;
171+
} else {
172+
process.env.BUILDKITE_BUILD_URL = originalBuildUrl;
173+
}
174+
});
175+
176+
it('renders a fallback body that points to the Buildkite build URL when set', () => {
177+
process.env.BUILDKITE_BUILD_URL = 'https://buildkite.com/org/pipeline/builds/123';
178+
const body = buildFailureBody(report({ status: 'fail', findings: [] }));
179+
180+
expect(body).toContain('Saved Objects CI check failed');
181+
expect(body).toContain('no structured findings were collected');
182+
expect(body).toContain('[Buildkite logs](https://buildkite.com/org/pipeline/builds/123)');
183+
expect(body).not.toMatch(/0 issue\(s\) across 0 type\(s\)/);
184+
});
185+
186+
it('falls back to a generic phrasing when BUILDKITE_BUILD_URL is unset', () => {
187+
delete process.env.BUILDKITE_BUILD_URL;
188+
const body = buildFailureBody(report({ status: 'fail', findings: [] }));
189+
190+
expect(body).toContain('See the Buildkite logs for details');
191+
expect(body).not.toContain('[Buildkite logs](');
192+
});
193+
});
194+
});

0 commit comments

Comments
 (0)