Skip to content

G
gitlab-ce
Commit 8a9cab53 authored by Yorick Peterse's avatar Yorick Peterse
Browse files
Options

Update tooling/docs for the new changelog workflow 

This updates the changelog documentation to cover the new changelog
workflow, and Danger to check for this new workflow correctly.
parent 34af4653
Hide whitespace changes
Inline Side-by-side
Showing with 210 additions and 906 deletions
bin/changelog deleted 100755 → 0
View file @ 34af4653
#!/usr/bin/env ruby
#
# Generate a changelog entry file in the correct location.
#
# Automatically stages the file and amends the previous commit if the `--amend`
# argument is used.
require 'optparse'
require 'yaml'
INVALID_TYPE = -1
module ChangelogHelpers
Abort = Class.new(StandardError)
Done = Class.new(StandardError)
MAX_FILENAME_LENGTH = 99 # GNU tar has a 99 character limit
def capture_stdout(cmd)
output = IO.popen(cmd, &:read)
fail_with "command failed: #{cmd.join(' ')}" unless $?.success?
output
end
def fail_with(message)
raise Abort, "\e[31merror\e[0m #{message}"
end
end
class ChangelogOptionParser
extend ChangelogHelpers
Options = Struct.new(
:amend,
:author,
:dry_run,
:force,
:merge_request,
:title,
:type,
:ee
)
Type = Struct.new(:name, :description)
TYPES = [
Type.new('added', 'New feature'),
Type.new('fixed', 'Bug fix'),
Type.new('changed', 'Feature change'),
Type.new('deprecated', 'New deprecation'),
Type.new('removed', 'Feature removal'),
Type.new('security', 'Security fix'),
Type.new('performance', 'Performance improvement'),
Type.new('other', 'Other')
].freeze
TYPES_OFFSET = 1
class << self
def parse(argv)
options = Options.new
parser = OptionParser.new do |opts|
opts.banner = "Usage: #{__FILE__} [options] [title]\n\n"
# Note: We do not provide a shorthand for this in order to match the `git
# commit` interface
opts.on('--amend', 'Amend the previous commit') do |value|
options.amend = value
end
opts.on('-f', '--force', 'Overwrite an existing entry') do |value|
options.force = value
end
opts.on('-m', '--merge-request [integer]', Integer, 'Merge request ID') do |value|
options.merge_request = value
end
opts.on('-n', '--dry-run', "Don't actually write anything, just print") do |value|
options.dry_run = value
end
opts.on('-u', '--git-username', 'Use Git user.name configuration as the author') do |value|
options.author = git_user_name if value
end
opts.on('-t', '--type [string]', String, "The category of the change, valid options are: #{TYPES.map(&:name).join(', ')}") do |value|
options.type = parse_type(value)
end
opts.on('-e', '--ee', 'Generate a changelog entry for GitLab EE') do |value|
options.ee = value
end
opts.on('-h', '--help', 'Print help message') do
$stdout.puts opts
raise Done.new
end
end
parser.parse!(argv)
# Title is everything that remains, but let's clean it up a bit
options.title = argv.join(' ').strip.squeeze(' ').tr("\r\n", '')
options
end
def read_type
read_type_message
type = TYPES[$stdin.getc.to_i - TYPES_OFFSET]
assert_valid_type!(type)
type.name
end
private
def parse_type(name)
type_found = TYPES.find do |type|
type.name == name
end
type_found ? type_found.name : INVALID_TYPE
end
def read_type_message
$stdout.puts "\n>> Please specify the index for the category of your change:"
TYPES.each_with_index do |type, index|
$stdout.puts "#{index + TYPES_OFFSET}. #{type.description}"
end
$stdout.print "\n?> "
end
def assert_valid_type!(type)
unless type
raise Abort, "Invalid category index, please select an index between 1 and #{TYPES.length}"
end
end
def git_user_name
capture_stdout(%w[git config user.name]).strip
end
end
end
class ChangelogEntry
include ChangelogHelpers
attr_reader :options
def initialize(options)
@options = options
end
def execute
assert_feature_branch!
assert_title! unless editor
assert_new_file!
# Read type from $stdin unless is already set
options.type ||= ChangelogOptionParser.read_type
assert_valid_type!
$stdout.puts "\e[32mcreate\e[0m #{file_path}"
$stdout.puts contents
unless options.dry_run
write
amend_commit if options.amend
end
if editor
system("#{editor} '#{file_path}'")
end
end
private
def contents
yaml_content = YAML.dump(
'title' => title,
'merge_request' => options.merge_request,
'author' => options.author,
'type' => options.type
)
remove_trailing_whitespace(yaml_content)
end
def write
File.write(file_path, contents)
end
def editor
ENV['EDITOR']
end
def amend_commit
fail_with "git add failed" unless system(*%W[git add #{file_path}])
Kernel.exec(*%w[git commit --amend])
end
def assert_feature_branch!
return unless branch_name == 'master'
fail_with "Create a branch first!"
end
def assert_new_file!
return unless File.exist?(file_path)
return if options.force
fail_with "#{file_path} already exists! Use `--force` to overwrite."
end
def assert_title!
return if options.title.length > 0 || options.amend
fail_with "Provide a title for the changelog entry or use `--amend`" \
" to use the title from the previous commit."
end
def assert_valid_type!
return unless options.type && options.type == INVALID_TYPE
fail_with 'Invalid category given!'
end
def title
if options.title.empty?
last_commit_subject
else
options.title
end
end
def last_commit_subject
capture_stdout(%w[git log --format=%s -1]).strip
end
def file_path
base_path = File.join(
unreleased_path,
branch_name.gsub(/[^\w-]/, '-'))
# Add padding for .yml extension
base_path[0..MAX_FILENAME_LENGTH - 5] + '.yml'
end
def unreleased_path
path = File.join('changelogs', 'unreleased')
path = File.join('ee', path) if ee?
path
end
def ee?
options.ee
end
def branch_name
@branch_name ||= capture_stdout(%w[git symbolic-ref --short HEAD]).strip
end
def remove_trailing_whitespace(yaml_content)
yaml_content.gsub(/ +$/, '')
end
end
if $0 == __FILE__
begin
options = ChangelogOptionParser.parse(ARGV)
ChangelogEntry.new(options).execute
rescue ChangelogHelpers::Abort => ex
$stderr.puts ex.message
exit 1
rescue ChangelogHelpers::Done
exit
end
end
# vim: ft=ruby
doc/development/changelog.md
View file @ 8a9cab53
......@@ -11,56 +11,84 @@ file, as well as information and history about our changelog process.
## Overview
Each bullet point, or **entry**, in our [`CHANGELOG.md`](https://gitlab.com/gitlab-org/gitlab/blob/master/CHANGELOG.md) file is
generated from a single data file in the [`changelogs/unreleased/`](https://gitlab.com/gitlab-org/gitlab/tree/master/changelogs/unreleased/).
The file is expected to be a [YAML](https://en.wikipedia.org/wiki/YAML) file in the
following format:
Each bullet point, or **entry**, in our
[`CHANGELOG.md`](https://gitlab.com/gitlab-org/gitlab/blob/master/CHANGELOG.md)
file is generated from the subject line of a Git commit. Commits are included
when they contain the `Changelog` [Git trailer](https://git-scm.com/docs/git-interpret-trailers).
When generating the changelog, author and merge request details are added
automatically.
```yaml
---
title: "Change[log]s"
merge_request: 1972
author: Black Sabbath @bsabbath
type: added
The `Changelog` trailer accepts the following values:
- added
- fixed
- changed
- deprecated
- removed
- security
- performance
- other
An example of a Git commit to include in the changelog is the following:
```plaintext
Update git vendor to gitlab
Now that we are using gitaly to compile git, the git version isn't known
from the manifest, instead we are getting the gitaly version. Update our
vendor field to be `gitlab` to avoid cve matching old versions.
Changelog: changed
```
### Overriding the associated merge request
GitLab automatically links the merge request to the commit when generating the
changelog. If you want to override the merge request to link to, you can specify
an alternative merge request using the `MR` trailer:
```plaintext
Update git vendor to gitlab
Now that we are using gitaly to compile git, the git version isn't known
from the manifest, instead we are getting the gitaly version. Update our
vendor field to be `gitlab` to avoid cve matching old versions.
Changelog: changed
MR: https://gitlab.com/foo/bar/-/merge_requests/123
```
The `merge_request` value is a reference to a merge request that adds this
entry, and the `author` key (format: `<full name> <GitLab username>`) is used to give attribution to community
contributors. **Both are optional**.
The `type` field maps the category of the change,
valid options are: added, fixed, changed, deprecated, removed, security, performance, other. **Type field is mandatory**.
The value must be the full URL of the merge request.
### GitLab Enterprise changes
If a change is for GitLab Enterprise Edition, you must also add the trailer `EE:
true`:
```plaintext
Update git vendor to gitlab
Now that we are using gitaly to compile git, the git version isn't known
from the manifest, instead we are getting the gitaly version. Update our
vendor field to be `gitlab` to avoid cve matching old versions.
Community contributors and core team members are encouraged to add their name to
the `author` field. GitLab team members **should not**.
Changelog: changed
MR: https://gitlab.com/foo/bar/-/merge_requests/123
EE: true
```
## What warrants a changelog entry?
- Any change that introduces a database migration, whether it's regular, post,
or data migration, **must** have a changelog entry, even if it is behind a
disabled feature flag. Since the migration is executed on [GitLab FOSS](https://gitlab.com/gitlab-org/gitlab-foss/),
the changelog for database schema changes should be written to the
`changelogs/unreleased/` directory, even when other elements of that change affect only GitLab EE.
- [Security fixes](https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md)
**must** have a changelog entry, without `merge_request` value
and with `type` set to `security`.
- Any user-facing change **must** have a changelog entry. This includes both visual changes (regardless of how minor), and changes to the rendered DOM which impact how a screen reader may announce the content.
- Any client-facing change to our REST and GraphQL APIs **must** have a changelog entry. See the [complete list what comprises a GraphQL breaking change](api_graphql_styleguide.md#breaking-changes).
- Any change that introduces an [Advanced Search migration](elasticsearch.md#creating-a-new-advanced-search-migration) **must** have a changelog entry.
- Performance improvements **should** have a changelog entry.
- _Any_ contribution from a community member, no matter how small, **may** have
a changelog entry regardless of these guidelines if the contributor wants one.
Example: "Fixed a typo on the search results page."
- Any docs-only changes **should not** have a changelog entry.
- For changes related to feature flags, use [feature flag guide](feature_flags/index.md#changelog) to determine the changelog entry.
- Any change that adds new Usage Data metrics, sets the status of existing ones to `removed`, and changes that need to be documented in Product Intelligence [Metrics Dictionary](usage_ping/dictionary.md) **should** have a changelog entry.
- A change that adds snowplow events **should** have a changelog entry -
- A fix for a regression introduced and then fixed in the same release (i.e.,
- Any user-facing change **should** have a changelog entry. Example: "GitLab now
uses system fonts for all text."
- A fix for a regression introduced and then fixed in the same release (such as
fixing a bug introduced during a monthly release candidate) **should not**
have a changelog entry.
- Any developer-facing change (e.g., refactoring, technical debt remediation,
test suite changes) **should not** have a changelog entry. Example: "Reduce
- Any developer-facing change (such as refactoring, technical debt remediation,
or test suite changes) **should not** have a changelog entry. Example: "Reduce
database records created during Cycle Analytics model spec."
- _Any_ contribution from a community member, no matter how small, **may** have
a changelog entry regardless of these guidelines if the contributor wants one.
## Writing good changelog entries
......@@ -105,215 +133,49 @@ about _where_ and _why_ the change was made?
## How to generate a changelog entry
A `bin/changelog` script is available to generate the changelog entry file
automatically.
Its simplest usage is to provide the value for `title`:
```plaintext
bin/changelog 'Hey DZ, I added a feature to GitLab!'
```
If you want to generate a changelog entry for GitLab EE, you must pass
the `--ee` option:
```plaintext
bin/changelog --ee 'Hey DZ, I added a feature to GitLab!'
```
All entries in the `CHANGELOG.md` file apply to all editions of GitLab.
Changelog updates are based on a common [GitLab codebase](https://gitlab.com/gitlab-org/gitlab/),
and are mirrored without proprietary code to [GitLab FOSS](https://gitlab.com/gitlab-org/gitlab-foss/) (also known as GitLab Community Edition).
At this point the script would ask you to select the category of the change (mapped to the `type` field in the entry):
```plaintext
>> Please specify the category of your change:
1. New feature
2. Bug fix
3. Feature change
4. New deprecation
5. Feature removal
6. Security fix
7. Performance improvement
8. Other
```
The entry filename is based on the name of the current Git branch. If you run
the command above on a branch called `feature/hey-dz`, it generates a
`changelogs/unreleased/feature-hey-dz.yml` file.
Git trailers are added when committing your changes. This can be done using your
text editor of choice. Adding the trailer to an existing commit requires either
amending to the commit (if it's the most recent one), or an interactive rebase
using `git rebase -i`.
The command outputs the path of the generated file and its contents:
To update the last commit, run the following:
```plaintext
create changelogs/unreleased/my-feature.yml
---
title: Hey DZ, I added a feature to GitLab!
merge_request:
author:
type:
```
### Arguments
| Argument | Shorthand | Purpose |
| ----------------- | --------- | --------------------------------------------------------------------------------------------------------------------------------------- |
| [`--amend`](#--amend) | | Amend the previous commit |
| [`--force`](#--force-or--f) | `-f` | Overwrite an existing entry |
| [`--merge-request`](#--merge-request-or--m) | `-m` | Set merge request ID |
| [`--dry-run`](#--dry-run-or--n) | `-n` | Don't actually write anything, just print |
| [`--git-username`](#--git-username-or--u) | `-u` | Use Git user.name configuration as the author |
| [`--type`](#--type-or--t) | `-t` | The category of the change, valid options are: `added`, `fixed`, `changed`, `deprecated`, `removed`, `security`, `performance`, `other` |
| [`--ee`](#how-to-generate-a-changelog-entry) | | Create an EE changelog
| `--help` | `-h` | Print help message |
#### `--amend`
You can pass the **`--amend`** argument to automatically stage the generated
file and amend it to the previous commit.
If you use **`--amend`** and don't provide a title, it uses
the "subject" of the previous commit, which is the first line of the commit
message:
```plaintext
$ git show --oneline
ab88683 Added an awesome new feature to GitLab
$ bin/changelog --amend
create changelogs/unreleased/feature-hey-dz.yml
---
title: Added an awesome new feature to GitLab
merge_request:
author:
type:
```
#### `--force` or `-f`
Use **`--force`** or **`-f`** to overwrite an existing changelog entry if it
already exists.
```plaintext
$ bin/changelog 'Hey DZ, I added a feature to GitLab!'
error changelogs/unreleased/feature-hey-dz.yml already exists! Use `--force` to overwrite.
$ bin/changelog 'Hey DZ, I added a feature to GitLab!' --force
create changelogs/unreleased/feature-hey-dz.yml
---
title: Hey DZ, I added a feature to GitLab!
merge_request: 1983
author:
type:
```
#### `--merge-request` or `-m`
Use the **`--merge-request`** or **`-m`** argument to provide the
`merge_request` value:
```plaintext
$ bin/changelog 'Hey DZ, I added a feature to GitLab!' -m 1983
create changelogs/unreleased/feature-hey-dz.yml
---
title: Hey DZ, I added a feature to GitLab!
merge_request: 1983
author:
type:
```shell
git commit --amend
```
#### `--dry-run` or `-n`
You can then add the `Changelog` trailer to the commit message. If you had
already pushed prior commits to your remote branch, you have to force push
the new commit:
Use the **`--dry-run`** or **`-n`** argument to prevent actually writing or
committing anything:
```plaintext
$ bin/changelog --amend --dry-run
create changelogs/unreleased/feature-hey-dz.yml
---
title: Added an awesome new feature to GitLab
merge_request:
author:
type:
$ ls changelogs/unreleased/
```shell
git push -f origin your-branch-name
```
#### `--git-username` or `-u`
Use the **`--git-username`** or **`-u`** argument to automatically fill in the
`author` value with your configured Git `user.name` value:
To edit older (or multiple commits), use `git rebase -i HEAD~N` where `N` is the
last N number of commits to rebase. Let's say you have 3 commits on your branch:
A, B, and C. If you want to update commit B, you need to run:
```plaintext
$ git config user.name
Jane Doe
$ bin/changelog -u 'Hey DZ, I added a feature to GitLab!'
create changelogs/unreleased/feature-hey-dz.yml
---
title: Hey DZ, I added a feature to GitLab!
merge_request:
author: Jane Doe
type:
```shell
git rebase -i HEAD~2
```
#### `--type` or `-t`
Use the **`--type`** or **`-t`** argument to provide the `type` value:
This starts an interactive rebase session for the last two commits. When
started, Git presents you with a text editor with contents along the lines of
the following:
```plaintext
$ bin/changelog 'Hey DZ, I added a feature to GitLab!' -t added
create changelogs/unreleased/feature-hey-dz.yml
---
title: Hey DZ, I added a feature to GitLab!
merge_request:
author:
type: added
pick B Subject of commit B
pick C Subject of commit C
```
#### `--ee`
Use the **`--ee`** argument to create an EE changelog:
```plaintext
$ bin/changelog 'Hey DZ, I added a feature to GitLab!' -ee
create ee/changelogs/unreleased/feature-hey-dz.yml
---
title: Hey DZ, I added a feature to GitLab!
merge_request:
author:
type: added
```
To update commit B, change the word `pick` to `reword`, then save and quit the
editor. Once closed, Git presents you with a new text editor instance to edit
the commit message of commit B. Add the trailer, then save and quit the editor.
If all went well, commit B is now updated.
### History and Reasoning
Our `CHANGELOG` file was previously updated manually by each contributor that
felt their change warranted an entry. When two merge requests added their own
entries at the same spot in the list, it created a merge conflict in one as soon
as the other was merged. When we had dozens of merge requests fighting for the
same changelog entry location, this quickly became a major source of merge
conflicts and delays in development.
This led us to a [boring solution](https://about.gitlab.com/handbook/values/#boring-solutions) of "add your entry in a random location in
the list." This actually worked pretty well as we got further along in each
monthly release cycle, but at the start of a new cycle, when a new version
section was added and there were fewer places to "randomly" add an entry, the
conflicts became a problem again until we had a sufficient number of entries.
On top of all this, it created an entirely different headache for
[release managers](https://gitlab.com/gitlab-org/release/docs/blob/master/quickstart/release-manager.md)
when they cherry-picked a commit into a stable branch for a patch release. If
the commit included an entry in the `CHANGELOG`, it would include the entire
changelog for the latest version in `master`, so the release manager would have
to manually remove the later entries. They often would have had to do this
multiple times per patch release. This was compounded when we had to release
multiple patches at once due to a security issue.
We needed to automate all of this manual work. So we
[started brainstorming](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/17826).
After much discussion we settled on the current solution of one file per entry,
and then compiling the entries into the overall `CHANGELOG.md` file during the
[release process](https://gitlab.com/gitlab-org/release-tools).
For more information about interactive rebases, take a look at [the Git
documentation](https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History).
---
......
doc/development/usage_ping/index.md
View file @ 8a9cab53
......@@ -1014,7 +1014,7 @@ Check if new metrics need to be added to the Versions Application. See `usage_da
Add the `feature` label to the Merge Request for new Usage Ping metrics. These are user-facing changes and are part of expanding the Usage Ping feature.
### 8. Add a changelog file
### 8. Add a changelog
Ensure you comply with the [Changelog entries guide](../changelog.md).
......
spec/bin/changelog_spec.rb deleted 100644 → 0
View file @ 34af4653
# frozen_string_literal: true
require 'spec_helper'
load File.expand_path('../../bin/changelog', __dir__)
RSpec.describe 'bin/changelog' do
let(:options) { OpenStruct.new(title: 'Test title', type: 'fixed', dry_run: true) }
describe ChangelogEntry do
it 'truncates the file path' do
entry = described_class.new(options)
allow(entry).to receive(:ee?).and_return(false)
allow(entry).to receive(:branch_name).and_return('long-branch-' * 100)
file_path = entry.send(:file_path)
expect(file_path.length).to eq(99)
end
end
describe ChangelogOptionParser do
describe '.parse' do
it 'parses --amend' do
options = described_class.parse(%w[foo bar --amend])
expect(options.amend).to eq true
end
it 'parses --force and -f' do
%w[--force -f].each do |flag|
options = described_class.parse(%W[foo #{flag} bar])
expect(options.force).to eq true
end
end
it 'parses --merge-request and -m' do
%w[--merge-request -m].each do |flag|
options = described_class.parse(%W[foo #{flag} 1234 bar])
expect(options.merge_request).to eq 1234
end
end
it 'parses --dry-run and -n' do
%w[--dry-run -n].each do |flag|
options = described_class.parse(%W[foo #{flag} bar])
expect(options.dry_run).to eq true
end
end
it 'parses --git-username and -u' do
allow(described_class).to receive(:git_user_name).and_return('Jane Doe')
%w[--git-username -u].each do |flag|
options = described_class.parse(%W[foo #{flag} bar])
expect(options.author).to eq 'Jane Doe'
end
end
it 'parses --type and -t' do
%w[--type -t].each do |flag|
options = described_class.parse(%W[foo #{flag} security])
expect(options.type).to eq 'security'
end
end
it 'parses --ee and -e' do
%w[--ee -e].each do |flag|
options = described_class.parse(%W[foo #{flag} security])
expect(options.ee).to eq true
end
end
it 'parses -h' do
expect do
expect { described_class.parse(%w[foo -h bar]) }.to output.to_stdout
end.to raise_error(ChangelogHelpers::Done)
end
it 'assigns title' do
options = described_class.parse(%W[foo -m 1 bar\n baz\r\n --amend])
expect(options.title).to eq 'foo bar baz'
end
end
describe '.read_type' do
let(:type) { '1' }
it 'reads type from $stdin' do
expect($stdin).to receive(:getc).and_return(type)
expect do
expect(described_class.read_type).to eq('added')
end.to output.to_stdout
end
context 'invalid type given' do
let(:type) { '99' }
it 'shows error message and exits the program' do
allow($stdin).to receive(:getc).and_return(type)
expect do
expect { described_class.read_type }.to raise_error(
ChangelogHelpers::Abort,
'Invalid category index, please select an index between 1 and 8'
)
end.to output.to_stdout
end
end
end
end
end
spec/tooling/danger/changelog_spec.rb
View file @ 8a9cab53
......@@ -18,20 +18,34 @@ RSpec.describe Tooling::Danger::Changelog do
allow(changelog).to receive(:project_helper).and_return(fake_project_helper)
end
describe '#check_changelog_trailer' do
subject { changelog.check_changelog_trailer(commit) }
describe '#check_changelog_commit_categories' do
context 'when all changelog commits are correct' do
it 'does not produce any messages' do
commit = double(:commit, message: "foo\nChangelog: fixed")
allow(changelog).to receive(:changelog_commits).and_return([commit])
context "when commit doesn't include a changelog trailer" do
let(:commit) { double('commit', message: "Hello world") }
expect(changelog).not_to receive(:fail)
it { is_expected.to be_nil }
changelog.check_changelog_commit_categories
end
end
context "when commit include a changelog trailer with no category" do
let(:commit) { double('commit', message: "Hello world\n\nChangelog:") }
context 'when a commit has an incorrect trailer' do
it 'adds a message' do
commit = double(:commit, message: "foo\nChangelog: foo", sha: '123')
allow(changelog).to receive(:changelog_commits).and_return([commit])
it { is_expected.to be_nil }
expect(changelog).to receive(:fail)
changelog.check_changelog_commit_categories
end
end
end
describe '#check_changelog_trailer' do
subject { changelog.check_changelog_trailer(commit) }
context "when commit include a changelog trailer with an unknown category" do
let(:commit) { double('commit', message: "Hello world\n\nChangelog: foo", sha: "abc123") }
......@@ -48,110 +62,6 @@ RSpec.describe Tooling::Danger::Changelog do
end
end
describe '#check_changelog_yaml' do
let(:changelog_path) { 'ee/changelogs/unreleased/entry.yml' }
let(:changes) { changes_class.new([change_class.new(changelog_path, :added, :changelog)]) }
let(:yaml_title) { 'Fix changelog Dangerfile to convert MR IID to a string before comparison' }
let(:yaml_merge_request) { 60899 }
let(:mr_iid) { '60899' }
let(:yaml_type) { 'fixed' }
let(:yaml) do
<<~YAML
---
title: #{yaml_title}
merge_request: #{yaml_merge_request}
author:
type: #{yaml_type}
YAML
end
before do
allow(changelog).to receive(:present?).and_return(true)
allow(changelog).to receive(:changelog_path).and_return(changelog_path)
allow(changelog).to receive(:read_file).with(changelog_path).and_return(yaml)
allow(fake_helper).to receive(:security_mr?).and_return(false)
allow(fake_helper).to receive(:mr_iid).and_return(mr_iid)
allow(fake_helper).to receive(:cherry_pick_mr?).and_return(false)
allow(fake_helper).to receive(:stable_branch?).and_return(false)
allow(fake_helper).to receive(:html_link).with(changelog_path).and_return(changelog_path)
end
subject { changelog.check_changelog_yaml }
context "when changelog is not present" do
before do
allow(changelog).to receive(:present?).and_return(false)
end
it { is_expected.to have_attributes(errors: [], warnings: [], markdowns: [], messages: []) }
end
context "when YAML is invalid" do
let(:yaml) { '{ foo bar]' }
it { is_expected.to have_attributes(errors: ["#{changelog_path} isn't valid YAML! #{described_class::SEE_DOC}"]) }
end
context "when a StandardError is raised" do
before do
allow(changelog).to receive(:read_file).and_raise(StandardError, "Fail!")
end
it { is_expected.to have_attributes(warnings: ["There was a problem trying to check the Changelog. Exception: StandardError - Fail!"]) }
end
context "when YAML title is nil" do
let(:yaml_title) { '' }
it { is_expected.to have_attributes(errors: ["`title` should be set, in #{changelog_path}! #{described_class::SEE_DOC}"]) }
end
context "when YAML type is nil" do
let(:yaml_type) { '' }
it { is_expected.to have_attributes(errors: ["`type` should be set, in #{changelog_path}! #{described_class::SEE_DOC}"]) }
end
context "when on a security MR" do
let(:yaml_merge_request) { '' }
before do
allow(fake_helper).to receive(:security_mr?).and_return(true)
end
it { is_expected.to have_attributes(errors: [], warnings: [], markdowns: [], messages: []) }
end
context "when MR IID is empty" do
before do
allow(fake_helper).to receive(:mr_iid).and_return("")
end
it { is_expected.to have_attributes(errors: [], warnings: [], markdowns: [], messages: []) }
end
context "when YAML MR IID is empty" do
let(:yaml_merge_request) { '' }
context "and YAML includes a merge_request: line" do
it { is_expected.to have_attributes(markdowns: [{ msg: format(described_class::SUGGEST_MR_COMMENT, mr_iid: fake_helper.mr_iid), file: changelog_path, line: 3 }]) }
end
context "and YAML does not include a merge_request: line" do
let(:yaml) do
<<~YAML
---
title: #{yaml_title}
author:
type: #{yaml_type}
YAML
end
it { is_expected.to have_attributes(messages: ["Consider setting `merge_request` to #{mr_iid} in #{changelog_path}. #{described_class::SEE_DOC}"]) }
end
end
end
describe '#check_changelog_path' do
let(:changelog_path) { 'changelog-path.yml' }
let(:foss_change) { nil }
......@@ -177,24 +87,25 @@ RSpec.describe Tooling::Danger::Changelog do
let(:ee_change) { change_class.new('ee/app/models/foo.rb', :added, :backend) }
context "and a non-EE changelog, and changelog not required" do
let(:changelog_change) { change_class.new('changelogs/unreleased/entry.yml', :added, :changelog) }
before do
allow(changelog).to receive(:required?).and_return(false)
allow(changelog).to receive(:ee_changelog?).and_return(false)
end
it { is_expected.to have_attributes(warnings: ["This MR has a Changelog file outside `ee/`, but code changes in `ee/`. Consider moving the Changelog file into `ee/`."]) }
it { is_expected.to have_attributes(warnings: ["This MR changes code in `ee/`, but is missing a Changelog commit. Consider adding the Changelog trailer to at least one commit."]) }
end
context "and a EE changelog" do
let(:changelog_change) { change_class.new('ee/changelogs/unreleased/entry.yml', :added, :changelog) }
before do
allow(changelog).to receive(:ee_changelog?).and_return(true)
end
it { is_expected.to have_attributes(errors: [], warnings: [], markdowns: [], messages: []) }
context "and there are DB changes" do
let(:foss_change) { change_class.new('db/migrate/foo.rb', :added, :migration) }
it { is_expected.to have_attributes(warnings: ["This MR has a Changelog file inside `ee/`, but there are database changes which [requires](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry) the Changelog placement to be outside of `ee/`. Consider moving the Changelog file outside `ee/`."]) }
it { is_expected.to have_attributes(warnings: ["This MR has a Changelog commit with the `EE: true` trailer, but there are database changes which [requires](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry) the Changelog commiot to not have the `EE: true` trailer. Consider removing the `EE: true` trailer."]) }
end
end
end
......@@ -203,15 +114,19 @@ RSpec.describe Tooling::Danger::Changelog do
let(:foss_change) { change_class.new('app/models/foo.rb', :added, :backend) }
context "and a non-EE changelog" do
let(:changelog_change) { change_class.new('changelogs/unreleased/entry.yml', :added, :changelog) }
before do
allow(changelog).to receive(:ee_changelog?).and_return(false)
end
it { is_expected.to have_attributes(errors: [], warnings: [], markdowns: [], messages: []) }
end
context "and a EE changelog" do
let(:changelog_change) { change_class.new('ee/changelogs/unreleased/entry.yml', :added, :changelog) }
before do
allow(changelog).to receive(:ee_changelog?).and_return(true)
end
it { is_expected.to have_attributes(warnings: ["This MR has a Changelog file in `ee/`, but no code changes in `ee/`. Consider moving the Changelog file outside `ee/`."]) }
it { is_expected.to have_attributes(warnings: ["This MR has a Changelog commit for EE, but no code changes in `ee/`. Consider removing the use of the `EE: true` trailer from your commits."]) }
end
end
end
......@@ -325,50 +240,63 @@ RSpec.describe Tooling::Danger::Changelog do
end
describe '#present?' do
subject { changelog.present? }
context 'added files contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :changelog)]) }
it 'returns true when a Changelog commit is present' do
allow(changelog)
.to receive(:valid_changelog_commits)
.and_return([double(:commit)])
it { is_expected.to be_truthy }
expect(changelog).to be_present
end
context 'added files do not contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :backend)]) }
it 'returns false when a Changelog commit is missing' do
allow(changelog).to receive(:valid_changelog_commits).and_return([])
it { is_expected.to be_falsy }
expect(changelog).not_to be_present
end
end
describe '#ee_changelog?' do
subject { changelog.ee_changelog? }
describe '#changelog_commits' do
it 'returns the commits that include a Changelog trailer' do
commit1 = double(:commit, message: "foo\nChangelog: fixed")
commit2 = double(:commit, message: "bar\nChangelog: kittens")
commit3 = double(:commit, message: 'testing')
git = double(:git)
context 'is ee changelog' do
let(:changes) { changes_class.new([change_class.new('ee/changelogs/unreleased/entry.yml', :added, :changelog)]) }
allow(changelog).to receive(:git).and_return(git)
allow(git).to receive(:commits).and_return([commit1, commit2, commit3])
it { is_expected.to be_truthy }
expect(changelog.changelog_commits).to eq([commit1, commit2])
end
end
describe '#valid_changelog_commits' do
it 'returns the commits with a valid Changelog trailer' do
commit1 = double(:commit, message: "foo\nChangelog: fixed")
commit2 = double(:commit, message: "bar\nChangelog: kittens")
context 'is not ee changelog' do
let(:changes) { changes_class.new([change_class.new('changelogs/unreleased/entry.yml', :added, :changelog)]) }
allow(changelog)
.to receive(:changelog_commits)
.and_return([commit1, commit2])
it { is_expected.to be_falsy }
expect(changelog.valid_changelog_commits).to eq([commit1])
end
end
describe '#changelog_path' do
subject { changelog.changelog_path }
describe '#ee_changelog?' do
it 'returns true when an EE changelog commit is present' do
commit = double(:commit, message: "foo\nEE: true")
context 'added files contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :changelog)]) }
allow(changelog).to receive(:changelog_commits).and_return([commit])
it { is_expected.to eq('foo') }
expect(changelog.ee_changelog?).to eq(true)
end
context 'added files do not contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :backend)]) }
it 'returns false when an EE changelog commit is missing' do
commit = double(:commit, message: 'foo')
allow(changelog).to receive(:changelog_commits).and_return([commit])
it { is_expected.to be_nil }
expect(changelog.ee_changelog?).to eq(false)
end
end
......@@ -379,8 +307,8 @@ RSpec.describe Tooling::Danger::Changelog do
shared_examples 'changelog modified text' do |key|
specify do
expect(subject).to include('CHANGELOG.md was edited')
expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
expect(subject).to include('`Changelog` trailer')
expect(subject).to include('`EE: true`')
end
end
......@@ -410,7 +338,7 @@ RSpec.describe Tooling::Danger::Changelog do
specify do
expect(subject).to include('CHANGELOG.md was edited')
expect(subject).not_to include('bin/changelog')
expect(subject).not_to include('`Changelog` trailer')
end
end
end
......@@ -429,8 +357,7 @@ RSpec.describe Tooling::Danger::Changelog do
specify do
expect(subject).to have_key(key)
expect(subject[key]).to include('CHANGELOG missing')
expect(subject[key]).to include('bin/changelog -m 1234 "Fake Title"')
expect(subject[key]).not_to include('--ee')
expect(subject[key]).to include('`Changelog` trailer')
end
end
......@@ -464,8 +391,7 @@ RSpec.describe Tooling::Danger::Changelog do
specify do
expect(subject).to have_key(key)
expect(subject[key]).to include('CHANGELOG missing')
expect(subject[key]).not_to include('bin/changelog')
expect(subject[key]).not_to include('--ee')
expect(subject[key]).not_to include('`Changelog` trailer')
end
end
......@@ -498,8 +424,8 @@ RSpec.describe Tooling::Danger::Changelog do
shared_examples 'changelog optional text' do |key|
specify do
expect(subject).to include('CHANGELOG missing')
expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
expect(subject).to include('`Changelog` trailer')
expect(subject).to include('EE: true')
end
end
......@@ -529,7 +455,6 @@ RSpec.describe Tooling::Danger::Changelog do
specify do
expect(subject).to include('CHANGELOG missing')
expect(subject).not_to include('bin/changelog')
end
end
end
......
tooling/danger/changelog.rb
View file @ 8a9cab53
......@@ -14,55 +14,21 @@ module Tooling
].freeze
NO_CHANGELOG_CATEGORIES = %i[docs none].freeze
CHANGELOG_TRAILER_REGEX = /^Changelog:\s*(?<category>.+)$/.freeze
CREATE_CHANGELOG_COMMAND = 'bin/changelog -m %<mr_iid>s "%<mr_title>s"'
CREATE_EE_CHANGELOG_COMMAND = 'bin/changelog --ee -m %<mr_iid>s "%<mr_title>s"'
CHANGELOG_EE_TRAILER_REGEX = /^EE: true$/.freeze
CHANGELOG_MODIFIED_URL_TEXT = "**CHANGELOG.md was edited.** Please remove the additions and follow the [changelog guidelines](https://docs.gitlab.com/ee/development/changelog.html).\n\n"
CHANGELOG_MISSING_URL_TEXT = "**[CHANGELOG missing](https://docs.gitlab.com/ee/development/changelog.html)**:\n\n"
OPTIONAL_CHANGELOG_MESSAGE = {
local: "If this merge request [doesn't need a CHANGELOG entry](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry), feel free to ignore this message.",
ci: <<~MSG
If you want to create a changelog entry for GitLab FOSS, run the following:
If you want to create a changelog entry for GitLab FOSS, add the `Changelog` trailer to the commit message you want to add to the changelog.
#{CREATE_CHANGELOG_COMMAND}
If you want to create a changelog entry for GitLab EE, run the following instead:
#{CREATE_EE_CHANGELOG_COMMAND}
If you want to create a changelog entry for GitLab EE, also add the trailer `EE: true` to your commit message.
If this merge request [doesn't need a CHANGELOG entry](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry), feel free to ignore this message.
MSG
}.freeze
CHANGELOG_NEW_WORKFLOW_MESSAGE = <<~MSG
We are in the process of rolling out a new workflow for adding changelog entries. This new workflow uses Git commit subjects and Git trailers to generate changelogs. This new approach will soon replace the current YAML based approach.
To ease the transition process, we recommend you start using both the old and new approach in parallel. This is not required at this time, but will make it easier to transition to the new approach in the future. To do so, pick the commit that should go in the changelog and add a `Changelog` trailer to it. For example:
```
This is my commit's subject line
This is the optional commit body.
Changelog: added
```
The value of the `Changelog` trailer should be one of the following: added, fixed, changed, deprecated, removed, security, performance, other.
For more information, take a look at the following resources:
- `https://gitlab.com/gitlab-com/gl-infra/delivery/-/issues/1564`
- https://docs.gitlab.com/ee/api/repositories.html#generate-changelog-data
If you'd like to see the new approach in action, take a look at the commits in [the Omnibus repository](https://gitlab.com/gitlab-org/omnibus-gitlab/-/commits/master).
MSG
SEE_DOC = "See the [changelog documentation](https://docs.gitlab.com/ee/development/changelog.html)."
SUGGEST_MR_COMMENT = <<~SUGGEST_COMMENT
```suggestion
merge_request: %<mr_iid>s
```
#{SEE_DOC}
SUGGEST_COMMENT
REQUIRED_CHANGELOG_REASONS = {
db_changes: 'introduces a database migration',
......@@ -71,9 +37,7 @@ module Tooling
REQUIRED_CHANGELOG_MESSAGE = {
local: "This merge request requires a changelog entry because it [%<reason>s](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry).",
ci: <<~MSG
To create a changelog entry, run the following:
#{CREATE_CHANGELOG_COMMAND}
To create a changelog entry, add the `Changelog` trailer to one of your Git commit messages.
This merge request requires a changelog entry because it [%<reason>s](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry).
MSG
......@@ -132,7 +96,6 @@ module Tooling
end
if present?
add_danger_messages(check_changelog_yaml)
add_danger_messages(check_changelog_path)
elsif required?
required_texts.each { |_, text| fail(text) } # rubocop:disable Lint/UnreachableLoop
......@@ -140,23 +103,7 @@ module Tooling
message optional_text
end
return unless helper.ci?
if required? || optional?
checked = 0
git.commits.each do |commit|
check_result = check_changelog_trailer(commit)
next if check_result.nil?
checked += 1
add_danger_messages(check_result)
end
if checked == 0
message CHANGELOG_NEW_WORKFLOW_MESSAGE
end
end
check_changelog_commit_categories
end
# rubocop:enable Style/SignalException
......@@ -169,11 +116,14 @@ module Tooling
end
# rubocop:enable Style/SignalException
def check_changelog_commit_categories
changelog_commits.each do |commit|
add_danger_messages(check_changelog_trailer(commit))
end
end
def check_changelog_trailer(commit)
trailer = commit.message.match(CHANGELOG_TRAILER_REGEX)
return if trailer.nil? || trailer[:category].nil?
category = trailer[:category]
return ChangelogCheckResult.empty if CATEGORIES.include?(category)
......@@ -181,62 +131,22 @@ module Tooling
ChangelogCheckResult.error("Commit #{commit.sha} uses an invalid changelog category: #{category}")
end
def check_changelog_yaml
check_result = ChangelogCheckResult.empty
return check_result unless present?
raw_file = read_file(changelog_path)
yaml = YAML.safe_load(raw_file)
yaml_merge_request = yaml["merge_request"].to_s
check_result.error("`title` should be set, in #{helper.html_link(changelog_path)}! #{SEE_DOC}") if yaml["title"].nil?
check_result.error("`type` should be set, in #{helper.html_link(changelog_path)}! #{SEE_DOC}") if yaml["type"].nil?
return check_result if helper.security_mr? || helper.mr_iid.empty?
check_changelog_yaml_merge_request(raw_file: raw_file, yaml_merge_request: yaml_merge_request, check_result: check_result)
rescue Psych::Exception
# YAML could not be parsed, fail the build.
ChangelogCheckResult.error("#{helper.html_link(changelog_path)} isn't valid YAML! #{SEE_DOC}")
rescue StandardError => e
ChangelogCheckResult.warning("There was a problem trying to check the Changelog. Exception: #{e.class.name} - #{e.message}")
end
def check_changelog_yaml_merge_request(raw_file:, yaml_merge_request:, check_result:)
cherry_pick_against_stable_branch = helper.cherry_pick_mr? && helper.stable_branch?
if yaml_merge_request.empty?
mr_line = raw_file.lines.find_index { |line| line =~ /merge_request:\s*\n/ }
if mr_line
check_result.markdown(msg: format(SUGGEST_MR_COMMENT, mr_iid: helper.mr_iid), file: changelog_path, line: mr_line.succ)
else
check_result.message("Consider setting `merge_request` to #{helper.mr_iid} in #{helper.html_link(changelog_path)}. #{SEE_DOC}")
end
elsif yaml_merge_request != helper.mr_iid && !cherry_pick_against_stable_branch
check_result.error("Merge request ID was not set to #{helper.mr_iid}! #{SEE_DOC}")
end
check_result
end
def check_changelog_path
check_result = ChangelogCheckResult.empty
return check_result unless present?
ee_changes = project_helper.all_ee_changes.dup
ee_changes.delete(changelog_path)
if ee_changes.any? && !ee_changelog? && !required?
check_result.warning("This MR has a Changelog file outside `ee/`, but code changes in `ee/`. Consider moving the Changelog file into `ee/`.")
check_result.warning("This MR changes code in `ee/`, but is missing a Changelog commit. Consider adding the Changelog trailer to at least one commit.")
end
if ee_changes.empty? && ee_changelog?
check_result.warning("This MR has a Changelog file in `ee/`, but no code changes in `ee/`. Consider moving the Changelog file outside `ee/`.")
check_result.warning("This MR has a Changelog commit for EE, but no code changes in `ee/`. Consider removing the use of the `EE: true` trailer from your commits.")
end
if ee_changes.any? && ee_changelog? && required_reasons.include?(:db_changes)
check_result.warning("This MR has a Changelog file inside `ee/`, but there are database changes which [requires](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry) the Changelog placement to be outside of `ee/`. Consider moving the Changelog file outside `ee/`.")
check_result.warning("This MR has a Changelog commit with the `EE: true` trailer, but there are database changes which [requires](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry) the Changelog commiot to not have the `EE: true` trailer. Consider removing the `EE: true` trailer.")
end
check_result
......@@ -258,33 +168,45 @@ module Tooling
end
def present?
!!changelog_path
valid_changelog_commits.any?
end
def ee_changelog?
changelog_path.start_with?('ee/')
def changelog_commits
git.commits.select do |commit|
commit.message.match?(CHANGELOG_TRAILER_REGEX)
end
end
def changelog_path
@changelog_path ||= project_helper.changes.added.by_category(:changelog).files.first
def valid_changelog_commits
changelog_commits.select do |commit|
trailer = commit.message.match(CHANGELOG_TRAILER_REGEX)
CATEGORIES.include?(trailer[:category])
end
end
def ee_changelog?
changelog_commits.any? do |commit|
commit.message.match?(CHANGELOG_EE_TRAILER_REGEX)
end
end
def modified_text
CHANGELOG_MODIFIED_URL_TEXT +
(helper.ci? ? format(OPTIONAL_CHANGELOG_MESSAGE[:ci], mr_iid: helper.mr_iid, mr_title: sanitized_mr_title) : OPTIONAL_CHANGELOG_MESSAGE[:local])
(helper.ci? ? format(OPTIONAL_CHANGELOG_MESSAGE[:ci]) : OPTIONAL_CHANGELOG_MESSAGE[:local])
end
def required_texts
required_reasons.each_with_object({}) do |required_reason, memo|
memo[required_reason] =
CHANGELOG_MISSING_URL_TEXT +
(helper.ci? ? format(REQUIRED_CHANGELOG_MESSAGE[:ci], reason: REQUIRED_CHANGELOG_REASONS.fetch(required_reason), mr_iid: helper.mr_iid, mr_title: sanitized_mr_title) : REQUIRED_CHANGELOG_MESSAGE[:local])
(helper.ci? ? format(REQUIRED_CHANGELOG_MESSAGE[:ci], reason: REQUIRED_CHANGELOG_REASONS.fetch(required_reason)) : REQUIRED_CHANGELOG_MESSAGE[:local])
end
end
def optional_text
CHANGELOG_MISSING_URL_TEXT +
(helper.ci? ? format(OPTIONAL_CHANGELOG_MESSAGE[:ci], mr_iid: helper.mr_iid, mr_title: sanitized_mr_title) : OPTIONAL_CHANGELOG_MESSAGE[:local])
(helper.ci? ? format(OPTIONAL_CHANGELOG_MESSAGE[:ci]) : OPTIONAL_CHANGELOG_MESSAGE[:local])
end
private
......@@ -293,10 +215,6 @@ module Tooling
File.read(path)
end
def sanitized_mr_title
Gitlab::Dangerfiles::TitleLinting.sanitize_mr_title(helper.mr_title)
end
def categories_need_changelog?
(project_helper.changes.categories - NO_CHANGELOG_CATEGORIES).any?
end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7