look for add of a changelog fragment in the whole PR rather than the last commit #772

quidame · 2021-03-21T20:55:42Z

Fixes #771

RATIONALE

The current ansible-changelog-fragment check leads to many false positives:

When a new commit is added and is not squashed with the one adding the changelog fragment.
When the pull-request contains a new plugin (module, filter, lookup...) and no changes of existing files.

This PR is a refactor of the whole check to look for add of a changelog fragment in the entire PR rather that just the last commit, and to not fail when a changelog fragment is not required.

quidame · 2021-04-13T12:36:00Z

@pabelanger could you take a look, please ?

Akasurde · 2021-04-14T04:34:29Z

@justjais @goneri @pabelanger Change looks good to me. Can you please review and merge? Thanks in advance.

goneri · 2021-04-14T14:19:52Z

Thank you @quidame for the change. May I ask you to move the logic in a new role. e.g ensure_changelog_fragment and ensure we don't use any zuul variables in the role.

include_role:
    name: ensure_changelog_fragment
vars:
   ensure_changelog_fragment_dir: "{{ zuul.executor.src_root }}/{{ zuul.project.canonical_name }}"
   ensure_changelog_fragment_default_branch: "{{ zuul.project.default-branch }}"

This way it will be way easier to test any future change and maybe, integrate that in the CI.

* Look for add of a changelog fragment in the whole PR rather than the last commit. * Also look for changes/removal of existing files and for add of a new plugin. Use results to mitigate the need of a changelog fragment.

goneri

If I use the main branch, I still get the "Your pull-request is missing a changelog fragment, please add one. It should explain to end users the reason for your change." error.

We should not have any error in this case IMO.

* add a 'git rebase' before 'git diff' queries

quidame · 2021-04-15T17:22:55Z

If I use the main branch, I still get the "Your pull-request is missing a changelog fragment, please add one. It should explain to end users the reason for your change." error.

Where ?

goneri · 2021-04-15T17:29:07Z

If I use the main branch, I still get the "Your pull-request is missing a changelog fragment, please add one. It should explain to end users the reason for your change." error.

Whe1re ?

I did this:

  tasks:
    - include_role:
        name: check_changelog_fragment
      vars:
        check_changelog_fragment__git_directory: /home/goneri/.ansible/collections/ansible_collections/amazon/aws

/home/goneri/.ansible/collections/ansible_collections/amazon/aws is an up to date git clone of amazon.aws and it's on the main branch.

goneri · 2021-04-15T17:30:44Z

playbooks/ansible-changelog-fragment/run.yaml

-
-    - name: Changelog fragment failed
-      fail:
-        msg: "Your pull-request is missing a changelog fragment, please add one. It should explain to end users the reason for your change."


I suggest to add this URL in the error message: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs

if the url is not expected to change for a couple of years, yes, I agree

pabelanger · 2021-04-15T17:38:59Z

playbooks/ansible-changelog-fragment/run.yaml

-    - name: Check for changelog fragments
-      args:
-        chdir: "{{ zuul.executor.src_root }}/{{ zuul.project.canonical_name }}"
-      shell: git show --name-status --exit-code --oneline --diff-filter=A | grep changelogs/fragments/


this is a pretty large refactor, I think you could just change this too

git show HEAD...origin/{{ zuul.branch }} --name-status --exit-code --oneline --diff-filter=A | grep changelogs/fragments/

And it will work as expected

this does not cover this:

You do not have to add a changelog fragment for PRs that add new modules and plugins, because our tooling does that for you automatically.

That is fine for now, the original purpose of this check was to enforce each commit to specific folders had the fragment. There shouldn't be harm for a user adding it in the same commit as a new module.

Also, a commit that only changes tests/ should not require a changelog fragment.

Also, a commit that only changes tests/ should not require a changelog fragment.

That is managed via https://github.com/ansible/project-config/blob/master/zuul.d/jobs.yaml#L76 if a project doesn't want them on tests, I would say create a new job, and parent to ansible-changelog-fragment updating the file matcher. In the case of network collections, changelogs for tests are still fine.

There shouldn't be harm for a user adding it in the same commit as a new module.

You're right, and in fact there is no harm for a user adding it in the same commit as a new module. But there shouldn't be harm for a user NOT adding it in the same PR/commit as a new module.

my feelings here are, that could be an optimization we make in a follow up patch. Even have a discussion on how we want to enhance this job. Your original issue was 'don't only check the last commit', which I think we can land now and test.

quidame · 2021-04-15T18:10:09Z

@goneri I don't understand your testcase. Do you play check_changelog_fragment against a repo where there is nothing to compare (main vs. main) ? What I understand is that the playbook fails because there is no changelog fragment, that is expected. The only reason why it would not complain about that is to have a new file in plugins directory... and 2 branches to get some diff between them.

goneri · 2021-04-15T18:39:07Z

@goneri I don't understand your testcase. Do you play check_changelog_fragment against a repo where there is nothing to compare (main vs. main) ? What I understand is that the playbook fails because there is no changelog fragment, that is expected. The only reason why it would not complain about that is to have a new file in plugins directory... and 2 branches to get some diff between them.

In my opinion, since their is no different between the two branches, we should just ignore the test. Sometime we run our tests again the main branches, e.g with periodical jobs. And in this case a role like this one should not raise an error.

goneri · 2021-04-15T19:51:06Z

roles/check_changelog_fragment/tasks/main.yml

+        check_changelog_fragment__new_plugin.rc > check_changelog_fragment__changed_files.rc
+    success_msg: >-
+      Your pull-request contains a new {{ 'changelog fragment' if
+      check_changelog_fragment__new_fragment.rc | bool else 'plugin' }}.


I also get the message when there is no difference between the two branches, but I'm fine with that at this stage.

goneri

Looks good to me, thank you @quidame.

quidame · 2021-04-15T20:26:24Z

@pabelanger @goneri thanks for reviewing !

goneri · 2021-04-15T20:30:20Z

@pabelanger @goneri thanks for reviewing !

You're welcome. Merci pour ta patience :-)

pabelanger · 2021-04-15T20:38:27Z

@quidame @goneri: given we cannot test this before merge, we should not modify this job. We should create a 2nd job, to iterate on, then merge it.

Because this run on the executor, we need to take special care about the git repos

pabelanger · 2021-04-15T20:39:34Z

Yes, this code looks to be broken:
https://3457d925ae4c684d0da8-9a76a141e01b81f7d0f477857486a1e5.ssl.cf2.rackcdn.com/201/5cb6296e014a4bfa82bbf1a641856913020023c4/check/ansible-changelog-fragment/6b77762/job-output.html#l78

Can we please revert, and create a new temp job?

goneri · 2021-04-15T20:40:25Z

Give me a sec, I push a revert PR.

goneri · 2021-04-15T21:07:07Z

This PR reverts the change on the run.yaml and provides a new job that consume the new role: #801

pabelanger · 2021-04-16T01:15:15Z

roles/check_changelog_fragment/tasks/main.yml

+#     * the PR renames existing file(s)
+
+
+# Rebasing ensures diffs are related to the PR, and only to the PR. If the task


I finally had some time to look at this, there isn't a need to rebase here. We have dedicated zuul-mergers that run merge operations, if the merge fails the job won't run. So, lets delete this.

ah, ok, that's fine, I'll remove it

pabelanger · 2021-04-16T01:16:09Z

roles/check_changelog_fragment/tasks/main.yml

+  changed_when: check_changelog_fragment__rebase.stdout_lines | length > 1
+
+
+- name: Look for any diff between current and target branches


there should always be a diff right? As the PR running will have a new commit?

yes, there should be a diff, except in the use case explained above by @goneri (playing the role in scheduled jobs against the default branch itself, so with an empty diff)

quidame force-pushed the fix/check_ansible-changelog-fragment_against_whole_pr branch 3 times, most recently from 323650e to 7e5fb52 Compare March 27, 2021 18:25

Akasurde mentioned this pull request Apr 14, 2021

Add 2.11 branch to test matrix, add ignore-2.12.txt ansible-collections/ansible.posix#168

Merged

gundalow requested a review from pabelanger April 14, 2021 12:33

refactor ansible-changelog-fragment check

86cc435

* Look for add of a changelog fragment in the whole PR rather than the last commit. * Also look for changes/removal of existing files and for add of a new plugin. Use results to mitigate the need of a changelog fragment.

quidame force-pushed the fix/check_ansible-changelog-fragment_against_whole_pr branch from 7e5fb52 to 97cddcd Compare April 15, 2021 16:35

goneri approved these changes Apr 15, 2021

View reviewed changes

goneri suggested changes Apr 15, 2021

View reviewed changes

move all tasks from playbook to a dedicated role

e1ec70a

* add a 'git rebase' before 'git diff' queries

quidame force-pushed the fix/check_ansible-changelog-fragment_against_whole_pr branch from 97cddcd to e1ec70a Compare April 15, 2021 17:14

goneri reviewed Apr 15, 2021

View reviewed changes

pabelanger reviewed Apr 15, 2021

View reviewed changes

apply @goneri suggestions (doc url + no change no fail)

5e5cfe3

goneri reviewed Apr 15, 2021

View reviewed changes

goneri approved these changes Apr 15, 2021

View reviewed changes

goneri requested a review from pabelanger April 15, 2021 19:56

goneri added the gate Gate PR in Zuul CI label Apr 15, 2021

ansible-zuul bot merged commit 1a7a95c into ansible:master Apr 15, 2021

quidame deleted the fix/check_ansible-changelog-fragment_against_whole_pr branch April 15, 2021 20:28

pabelanger reviewed Apr 16, 2021

View reviewed changes

quidame mentioned this pull request Apr 17, 2021

[BUG] role check_changelog_fragment does not work as expected #804

Closed

quidame mentioned this pull request Apr 30, 2021

search for a changelog fragment in the whole PR #812

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

look for add of a changelog fragment in the whole PR rather than the last commit #772

look for add of a changelog fragment in the whole PR rather than the last commit #772

quidame commented Mar 21, 2021 •

edited

Loading

quidame commented Apr 13, 2021

Akasurde commented Apr 14, 2021

goneri commented Apr 14, 2021

goneri left a comment

quidame commented Apr 15, 2021

goneri commented Apr 15, 2021

goneri Apr 15, 2021

quidame Apr 15, 2021

pabelanger Apr 15, 2021 •

edited

Loading

quidame Apr 15, 2021

pabelanger Apr 15, 2021

goneri Apr 15, 2021

pabelanger Apr 15, 2021

quidame Apr 15, 2021

pabelanger Apr 15, 2021

quidame commented Apr 15, 2021

goneri commented Apr 15, 2021

goneri Apr 15, 2021 •

edited

Loading

goneri left a comment

quidame commented Apr 15, 2021

goneri commented Apr 15, 2021

pabelanger commented Apr 15, 2021

pabelanger commented Apr 15, 2021

goneri commented Apr 15, 2021

goneri commented Apr 15, 2021

pabelanger Apr 16, 2021

quidame Apr 16, 2021

pabelanger Apr 16, 2021

quidame Apr 16, 2021

		# * the PR renames existing file(s)


		# Rebasing ensures diffs are related to the PR, and only to the PR. If the task

		changed_when: check_changelog_fragment__rebase.stdout_lines \| length > 1


		- name: Look for any diff between current and target branches

look for add of a changelog fragment in the whole PR rather than the last commit #772

look for add of a changelog fragment in the whole PR rather than the last commit #772

Conversation

quidame commented Mar 21, 2021 • edited Loading

RATIONALE

quidame commented Apr 13, 2021

Akasurde commented Apr 14, 2021

goneri commented Apr 14, 2021

goneri left a comment

Choose a reason for hiding this comment

quidame commented Apr 15, 2021

goneri commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabelanger Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quidame commented Apr 15, 2021

goneri commented Apr 15, 2021

goneri Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

goneri left a comment

Choose a reason for hiding this comment

quidame commented Apr 15, 2021

goneri commented Apr 15, 2021

pabelanger commented Apr 15, 2021

pabelanger commented Apr 15, 2021

goneri commented Apr 15, 2021

goneri commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quidame commented Mar 21, 2021 •

edited

Loading

pabelanger Apr 15, 2021 •

edited

Loading

goneri Apr 15, 2021 •

edited

Loading