nextchanges: Enforce changelog fragment format - #6395
Conversation
…nges-enforce-format
Integration test reportCommit: 6b463c6
Top 3 slowest tests (at least 2 minutes):
|
| - Create `.nextchanges/<section>/<name>.md`, picking the section folder that fits: `cli`, `bundles`, `dependency-updates`, `notable-changes`, or `api-changes`. `<name>` is arbitrary (a feature name or your PR number) — just keep it unique. | ||
| - Write one or two sentences in user-facing language, no Jira links. The leading `* ` is optional. Match the voice and tense of existing changelog entries. | ||
| - A PR link is optional: write `(#NNNN)` (with NNNN being the PR number) in the text and it's expanded to a full link automatically. | ||
| - Write a single line in user-facing language, no Jira links: start it with a `* ` bullet marker and end it with a period. Match the voice and tense of existing changelog entries. |
There was a problem hiding this comment.
Re: the voice and tense, this should be prescriptive because existing entries are ambiguous.
There was a problem hiding this comment.
True, but will do this as follow-up (unrelated to this change). Agents are too verbose in the entries currently...
| # is inferred from the commit that added it. | ||
| - name: Validate .nextchanges placement | ||
| run: uv run tools/validate_nextchanges.py | ||
| run: uv run tools/validate_nextchanges.py --strict ${{ github.event_name == 'pull_request' && format('--pr-number {0}', github.event.pull_request.number) || '' }} |
There was a problem hiding this comment.
This always fails on the first push. Could we e.g. not run this on drafts or something?
There was a problem hiding this comment.
This is a very cheap test (only takes a couple of seconds or so) so failing on draft actually gives you/the agent the signal that it will need an update before making this a proper PR. On the flip side, you can create a draft and have this skipped/pass, and once you do changes locally and run your local check it will pick up the PR and fail.
All that being sad, there's pros and cons and I'm ok to skip the workflow on draft but don't see the value right now (since there's no clear upside of using drafts, even integration tests run on draft; maybe something we can actually change too?)
| problems = find_problems(changelog_dir, sections) | ||
| # A trailing PR link is required whenever the change can be associated with a | ||
| # PR, and must name that PR. CI passes --strict (pull requests and pushes to | ||
| # main) so enforcement never fails open there, plus --pr-number for pull |
There was a problem hiding this comment.
What happens if we have to squash merge something without a changelog entry?
There was a problem hiding this comment.
If it doesn't have a changelog entry at all, this won't be enforced. We only enforce the PR link for new fragments.
| parser.add_argument( | ||
| "--strict", | ||
| action="store_true", | ||
| help="fail closed: require every fragment's trailing PR link even when the branch's PR can't be auto-detected (set in CI)", |
There was a problem hiding this comment.
do we ever need a non-strict mode?
There was a problem hiding this comment.
Yes, for failing open. But as you stated below, this can be inferred via the CI env variable.
| parser.add_argument( | ||
| "--pr-number", | ||
| default=None, | ||
| help="the PR under review; used as the expected link for not-yet-merged fragments (CI passes it for pull requests)", |
There was a problem hiding this comment.
Unclear what this does from the description. Especially give that we have code above to get current PR?
There was a problem hiding this comment.
basically there are three modes:
Local run, no PR yet: fail open on gh command
Local run, with PR: fail open on gh command, but if we can get PR, update in place
CI run: require link to be present, don't run PR number check.
CI can be detected via env var, no script argument necessary.
There was a problem hiding this comment.
but if we can get PR, update in place
not implemented, but this would be a nice addition!
CI run: don't run PR number check
do run the PR number check, because you might add the wrong link. That's what --pr-number {{ github.event.pull_request.number }} is for, to be authoritative and not infer (avoid having to handle multiple PRs from the same branch etc)
There was a problem hiding this comment.
That's what --pr-number {{ github.event.pull_request.number }} is for, to be authoritative and not infer (avoid having to handle multiple PRs from the same branch etc)
That makes sense!
not implemented, but this would be a nice addition!
can we have this? without it it's almost not worth it.
There was a problem hiding this comment.
can we have this? without it it's almost not worth it.
will do the auto-fix as a follow-up. I think it's still worth it to enforce the format in CI, even if not auto-fixable.
The best-effort `gh pr view` lookup swallowed OSError/SubprocessError silently, hiding why local PR detection did nothing. Print the error to stderr so it is visible. Co-authored-by: Isaac <no-reply@databricks.com>
…nges-enforce-format
…nges-enforce-format
Changes
tools/validate_nextchanges.pyto be more opinionated:*required#123numbers are expanded github links (issues and PRs)tools/update_github_links.pyand have all link validation be done intools/validate_nextchanges.py.github/workflows/changelog-preview.ymlworkflow (runs on PRs and pushes to main) does strict validation on the PR being added to new fragmentsWhy
We've been skipping PR numbers/links for a few releases. This enforces it.
Tests
Added 4 test.md files (see https://github.com/databricks/cli/tree/99d5458b3248482f21e2b5f12568bbd8d74d6c44/.nextchanges/cli)
Workflow validation of test files added to this PR:
When fixing the PR attribution in test3.md to 6395 locally and running
task changelog-previewit passes (because locally the test callsghand associates the branch to an open PR). If running locally before opening a PR (or quickly renaming branch) the test passes since we don't enforce PR link logic there.Valid entries look like this (
task changelog-preview):