Commit b3362cf7 authored by Albert Salim's avatar Albert Salim

Merge branch 'refactor-changelog-dangerfile' into 'master'

Move the changelog Dangerfile logic to a fully-tested helper module

See merge request gitlab-org/gitlab!60919
parents c145b823 fefe7cc4
# frozen_string_literal: true
# rubocop:disable Style/SignalException
require 'yaml'
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
CATEGORIES = YAML
.load_file(File.expand_path('../../.gitlab/changelog_config.yml', __dir__))
.fetch('categories')
.keys
.freeze
def check_changelog_trailer(commit)
trailer = commit.message.match(/^Changelog:\s*(?<category>.+)$/)
return :missing if trailer.nil? || trailer[:category].nil?
category = trailer[:category]
return :valid if CATEGORIES.include?(category)
self.fail(
"Commit #{commit.sha} uses an invalid changelog category: #{category}"
)
:invalid
end
def check_changelog_yaml(path)
raw_file = File.read(path)
yaml = YAML.safe_load(raw_file)
yaml_merge_request = yaml["merge_request"].to_s
fail "`title` should be set, in #{helper.html_link(path)}! #{SEE_DOC}" if yaml["title"].nil?
fail "`type` should be set, in #{helper.html_link(path)}! #{SEE_DOC}" if yaml["type"].nil?
return if helper.security_mr?
return if helper.mr_iid.empty?
cherry_pick_against_stable_branch = helper.cherry_pick_mr? && helper.stable_branch?
if yaml_merge_request.empty?
mr_line = raw_file.lines.find_index("merge_request:\n")
if mr_line
markdown(format(SUGGEST_MR_COMMENT, mr_iid: helper.mr_iid), file: path, line: mr_line.succ)
else
message "Consider setting `merge_request` to #{helper.mr_iid} in #{helper.html_link(path)}. #{SEE_DOC}"
end
elsif yaml_merge_request != helper.mr_iid && !cherry_pick_against_stable_branch
fail "Merge request ID was not set to #{helper.mr_iid}! #{SEE_DOC}"
end
rescue Psych::Exception
# YAML could not be parsed, fail the build.
fail "#{helper.html_link(path)} isn't valid YAML! #{SEE_DOC}"
rescue StandardError => e
warn "There was a problem trying to check the Changelog. Exception: #{e.class.name} - #{e.message}"
end
def check_changelog_path(path)
ee_changes = project_helper.all_ee_changes.dup
ee_changes.delete(path)
if ee_changes.any? && !changelog.ee_changelog? && !changelog.required?
warn "This MR has a Changelog file outside `ee/`, but code changes in `ee/`. Consider moving the Changelog file into `ee/`."
end
if ee_changes.empty? && changelog.ee_changelog?
warn "This MR has a Changelog file in `ee/`, but no code changes in `ee/`. Consider moving the Changelog file outside `ee/`."
end
if ee_changes.any? && changelog.ee_changelog? && changelog.required_reasons.include?(:db_changes)
warn "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/`."
end
end
if git.modified_files.include?("CHANGELOG.md")
fail changelog.modified_text
end
changelog_found = changelog.found
if changelog_found
check_changelog_yaml(changelog_found)
check_changelog_path(changelog_found)
elsif changelog.required?
changelog.required_texts.each { |_, text| fail(text) } # rubocop:disable Lint/UnreachableLoop
elsif changelog.optional?
message changelog.optional_text
end
if helper.ci? && (changelog.required? || changelog.optional?)
checked = 0
git.commits.each do |commit|
case check_changelog_trailer(commit)
when :valid, :invalid
checked += 1
end
end
if checked == 0
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
end
end
changelog.check!
......@@ -18,6 +18,204 @@ 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) }
context "when commit doesn't include a changelog trailer" do
let(:commit) { double('commit', message: "Hello world") }
it { is_expected.to be_nil }
end
context "when commit include a changelog trailer with no category" do
let(:commit) { double('commit', message: "Hello world\n\nChangelog:") }
it { is_expected.to be_nil }
end
context "when commit include a changelog trailer with an unknown category" do
let(:commit) { double('commit', message: "Hello world\n\nChangelog: foo", sha: "abc123") }
it { is_expected.to have_attributes(errors: ["Commit #{commit.sha} uses an invalid changelog category: foo"]) }
end
described_class::CATEGORIES.each do |category|
context "when commit include a changelog trailer with category set to '#{category}'" do
let(:commit) { double('commit', message: "Hello world\n\nChangelog: #{category}", sha: "abc123") }
it { is_expected.to have_attributes(errors: []) }
end
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 }
let(:ee_change) { nil }
let(:changelog_change) { nil }
let(:changes) { changes_class.new([foss_change, ee_change, changelog_change].compact) }
before do
allow(changelog).to receive(:present?).and_return(true)
end
subject { changelog.check_changelog_path }
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 "with EE changes" 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)
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/`."]) }
end
context "and a EE changelog" do
let(:changelog_change) { change_class.new('ee/changelogs/unreleased/entry.yml', :added, :changelog) }
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/`."]) }
end
end
end
context "with no EE changes" 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) }
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) }
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/`."]) }
end
end
end
describe '#required_reasons' do
subject { changelog.required_reasons }
......@@ -126,8 +324,8 @@ RSpec.describe Tooling::Danger::Changelog do
end
end
describe '#found' do
subject { changelog.found }
describe '#present?' do
subject { changelog.present? }
context 'added files contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :changelog)]) }
......@@ -138,7 +336,7 @@ RSpec.describe Tooling::Danger::Changelog do
context 'added files do not contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :backend)]) }
it { is_expected.to eq(nil) }
it { is_expected.to be_falsy }
end
end
......@@ -158,6 +356,22 @@ RSpec.describe Tooling::Danger::Changelog do
end
end
describe '#changelog_path' do
subject { changelog.changelog_path }
context 'added files contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :changelog)]) }
it { is_expected.to eq('foo') }
end
context 'added files do not contain a changelog' do
let(:changes) { changes_class.new([change_class.new('foo', :added, :backend)]) }
it { is_expected.to be_nil }
end
end
describe '#modified_text' do
subject { changelog.modified_text }
......
......@@ -256,7 +256,9 @@ RSpec.describe Tooling::Danger::ProjectHelper do
subject { project_helper.all_ee_changes }
it 'returns all changed files starting with ee/' do
expect(fake_helper).to receive(:all_changed_files).and_return(%w[fr/ee/beer.rb ee/wine.rb ee/lib/ido.rb ee.k])
changes = double
expect(project_helper).to receive(:changes).and_return(changes)
expect(changes).to receive(:files).and_return(%w[fr/ee/beer.rb ee/wine.rb ee/lib/ido.rb ee.k])
is_expected.to match_array(%w[ee/wine.rb ee/lib/ido.rb])
end
......
......@@ -13,9 +13,10 @@ module Tooling
'meta'
].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_MODIFIED_URL_TEXT = "**CHANGELOG.md was edited.** Please remove the additions and create a CHANGELOG entry.\n\n"
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 = {
......@@ -32,6 +33,36 @@ module Tooling
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',
......@@ -48,6 +79,169 @@ module Tooling
MSG
}.freeze
CATEGORIES = YAML
.load_file(File.expand_path('../../.gitlab/changelog_config.yml', __dir__))
.fetch('categories')
.keys
.freeze
class ChangelogCheckResult
attr_reader :errors, :warnings, :markdowns, :messages
def initialize(errors: [], warnings: [], markdowns: [], messages: [])
@errors = errors
@warnings = warnings
@markdowns = markdowns
@messages = messages
end
private_class_method :new
def self.empty
new
end
def self.error(error)
new(errors: [error])
end
def self.warning(warning)
new(warnings: [warning])
end
def error(error)
errors << error
end
def warning(warning)
warnings << warning
end
def markdown(markdown)
markdowns << markdown
end
def message(message)
messages << message
end
end
# rubocop:disable Style/SignalException
def check!
if git.modified_files.include?("CHANGELOG.md")
fail modified_text
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
elsif optional?
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
end
# rubocop:enable Style/SignalException
# rubocop:disable Style/SignalException
def add_danger_messages(check_result)
check_result.errors.each { |error| fail(error) } # rubocop:disable Lint/UnreachableLoop
check_result.warnings.each { |warning| warn(warning) }
check_result.markdowns.each { |markdown_hash| markdown(**markdown_hash) }
check_result.messages.each { |text| message(text) }
end
# rubocop:enable Style/SignalException
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)
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/`.")
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/`.")
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/`.")
end
check_result
end
def required_reasons
[].tap do |reasons|
reasons << :db_changes if project_helper.changes.added.has_category?(:migration)
......@@ -60,15 +254,19 @@ module Tooling
end
def optional?
categories_need_changelog? && without_no_changelog_label?
categories_need_changelog? && mr_without_no_changelog_label?
end
def found
@found ||= project_helper.changes.added.by_category(:changelog).files.first
def present?
!!changelog_path
end
def ee_changelog?
found.start_with?('ee/')
changelog_path.start_with?('ee/')
end
def changelog_path
@changelog_path ||= project_helper.changes.added.by_category(:changelog).files.first
end
def modified_text
......@@ -91,6 +289,10 @@ module Tooling
private
def read_file(path)
File.read(path)
end
def sanitized_mr_title
Gitlab::Dangerfiles::TitleLinting.sanitize_mr_title(helper.mr_title)
end
......@@ -99,7 +301,7 @@ module Tooling
(project_helper.changes.categories - NO_CHANGELOG_CATEGORIES).any?
end
def without_no_changelog_label?
def mr_without_no_changelog_label?
(helper.mr_labels & NO_CHANGELOG_LABELS).empty?
end
end
......
......@@ -168,7 +168,7 @@ module Tooling
end
def all_ee_changes
helper.all_changed_files.grep(%r{\Aee/})
changes.files.grep(%r{\Aee/})
end
def project_name
......
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