[BUG] role check_changelog_fragment does not work as expected

[quidame]

SUMMARY

The new changelog-fragment check, which is implemented here does not work as expected: it never fails.

This report follows up #771 , #772 and #800 to #803.
See also the bypassed check for this PR.

BEFORE

The previous check was something like

git show --oneline --name-status --exit-code --diff-filter=A -- changelogs/fragments/

It had an issue (#771) regarding the depth of the search (last commit), resulting in a additional workload for contributors and for the CI platform. For contributors, because once a changelog fragment has been added to a PR, any further change in this PR needs to be interactively rebased to squash last commits and the add of the changelog fragment altogether. For the CI platform, because tests are launched when a contributor applies a reviewer suggestion, and they fail consistently because applying the suggestion creates a new commit that obviouly does not contain the add of the changelog fragment, leading contributor to squash commits again and force-push to pass the tests.

NOW

In the new implementation, the git show command has been replaced by git diff {{ default_branch }} to search the add of the changelog fragment in the PR, i.e. in the difference between the default branch and the PR branch. If the diff is empty, the check is bypassed, allowing scheduled jobs to play this role without error.

Additionally, the check has been refactored to follow documentation and not fail for the add of a new plugin without a new changelog fragment (as long as other files have not been modified/deleted/renamed in the PR).

Unfortunately, this is based on the assumption that the git working tree is currently (checked out) on the PR branch, that is not the case at all ! It seems that when the role is played, the PR branch has already been merged into the default branch (within the testbed), resulting in an empty git diff output, and as such bypassing the check.

PLAN

git diff also works between commits. How to identify, retrieve and use the related commit to get the expected diff between this commit and HEAD ?

Another suggestion ?

[quidame]

help_needed

@pabelanger , could you confirm that at this step the PR branch is already merged into the default branch, so looking for *git diff default-branch` output doesn't make sense here ?

[pabelanger]

I am unsure why you are looking for the default branch, the check should be done again the current branch the PR is against. In this case, it is {{ zuul.branch }}.

As I said in my original comment (https://github.com/ansible/project-config/pull/772/files/e1ec70a364aca273c088e8d1bb389d43b6a95099#r614268838), I feel this check is overly complicated and grow beyond the original reported issue. I believe the following actually addresses the issue of multiple commits:

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

Can you please confirm, if so I propose we revert and do this for now. Then, we can work on addressing the other issue you raised about new modules.

[quidame]

I think all is doable at once, but if you prefer to split it and fix the "look in the whole PR" and the "no fragment for new plugin" step by step, why not.

I can't confirm the new git command you propose could solve the first issue. After a merge of PR-BRANCH into main, git show HEAD...origin/PR-BRANCH is logically empty, no ?

[quidame]

I don't think git show is the best command, anyway. git diff provides the same info with the same options, in a more concise and less confusing output:

$ git show HEAD...origin/new_module/filesize --name-status --oneline 
d4943cd8 (origin/new_module/filesize, new_module/filesize) doc: use strict lowercase booleans (true/false) rather than other variants
M       plugins/modules/files/filesize.py
1386e1d3 Update filesize.py (extends_documentation_fragment: use fqcn)
M       plugins/modules/files/filesize.py
68afb559 Update plugins/modules/files/filesize.py (version_added)
M       plugins/modules/files/filesize.py
a36b500d new module: filesize
A       plugins/modules/files/filesize.py
A       plugins/modules/filesize.py
A       tests/integration/targets/filesize/aliases
A       tests/integration/targets/filesize/defaults/main.yml
A       tests/integration/targets/filesize/tasks/basics.yml
A       tests/integration/targets/filesize/tasks/errors.yml
A       tests/integration/targets/filesize/tasks/floats.yml
A       tests/integration/targets/filesize/tasks/main.yml
A       tests/integration/targets/filesize/tasks/sparse.yml
A       tests/integration/targets/filesize/tasks/symlinks.yml

and

$ git diff HEAD...origin/new_module/filesize --name-status
A       plugins/modules/files/filesize.py
A       plugins/modules/filesize.py
A       tests/integration/targets/filesize/aliases
A       tests/integration/targets/filesize/defaults/main.yml
A       tests/integration/targets/filesize/tasks/basics.yml
A       tests/integration/targets/filesize/tasks/errors.yml
A       tests/integration/targets/filesize/tasks/floats.yml
A       tests/integration/targets/filesize/tasks/main.yml
A       tests/integration/targets/filesize/tasks/sparse.yml
A       tests/integration/targets/filesize/tasks/symlinks.yml

git show shows modifications of a file that has been added right before, where git diff shows the diff between two references, not all the intermediate objects.

The command outputs above are before merging new_module/filesize into main. After the merge of new_module/filesize into main, the two branches have the same content (that is the goal of the merge, isn't it ?), so I can't figure a way to get the content of the PR once its branch has been merged.

$ git merge new_module/filesize 
Mise à jour f4858d64..d4943cd8
Fast-forward
 plugins/modules/files/filesize.py                     | 470 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 plugins/modules/filesize.py                           |   1 +
 tests/integration/targets/filesize/aliases            |   1 +
 tests/integration/targets/filesize/defaults/main.yml  |   4 +
 tests/integration/targets/filesize/tasks/basics.yml   | 407 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/integration/targets/filesize/tasks/errors.yml   | 117 +++++++++++++++++++
 tests/integration/targets/filesize/tasks/floats.yml   | 245 +++++++++++++++++++++++++++++++++++++++
 tests/integration/targets/filesize/tasks/main.yml     |  40 +++++++
 tests/integration/targets/filesize/tasks/sparse.yml   | 282 ++++++++++++++++++++++++++++++++++++++++++++
 tests/integration/targets/filesize/tasks/symlinks.yml |  93 +++++++++++++++
 10 files changed, 1660 insertions(+)
 create mode 100644 plugins/modules/files/filesize.py
 create mode 120000 plugins/modules/filesize.py
 create mode 100644 tests/integration/targets/filesize/aliases
 create mode 100644 tests/integration/targets/filesize/defaults/main.yml
 create mode 100644 tests/integration/targets/filesize/tasks/basics.yml
 create mode 100644 tests/integration/targets/filesize/tasks/errors.yml
 create mode 100644 tests/integration/targets/filesize/tasks/floats.yml
 create mode 100644 tests/integration/targets/filesize/tasks/main.yml
 create mode 100644 tests/integration/targets/filesize/tasks/sparse.yml
 create mode 100644 tests/integration/targets/filesize/tasks/symlinks.yml

and:

$ git show HEAD...origin/new_module/filesize --name-status --oneline
$ git diff HEAD...origin/new_module/filesize --name-status

nothing + nothing

What we need is the diff between 2 references that are not the same in terms of content (if I create a new branch from main, I commit something and then revert the commit, git show will show the branches are different (2 commits), and git diff will show nothing (0 file)).

What we need is 2 references. Before the merge, they are easy to identify and use. After the merge, we have one: HEAD. Where is the other ? How to retrieve it to produce the name-status's digest we need ?

(Is it possible to run this check before zuul mergers ?)

[quidame]

@pabelanger I got it !

git diff origin/{{ default_branch }} HEAD --name-status --exit-code --diff-filter=A -- changelogs/fragments/

pr-branch, origin/pr-branch and main are in the same state, but main has been modified only on the testbed by zuul mergers, so origin/main still holds the old state (before the merge) and we can reliably compare main (or HEAD) with its origin/main.

How to test it ? (not the git command, the check within its context)

[quidame]

I'm not sure to understand. {{ zuul.branch }} is the name of the target branch, isn't it ? not the pr-branch, but main or master (in almost all cases) ?

[pabelanger]

Both are basically the same command:

(build) [pabelanger@pabelanger network-ee]$ git diff origin/main HEAD --name-status --exit-code --diff-filter=A -- changelogs/fragments/
A	changelogs/fragments/foo
(build) [pabelanger@pabelanger network-ee]$ git show origin/main...HEAD --name-status --exit-code --oneline --diff-filter=A -- changelogs/fragments
bd4b531 t
A       changelogs/fragments/foo

At this point, either is fine. And we can revert a bunch of the code for checking specifically for plugins.

As for default branch, we don't want this as PRs can be raised against non default branches. {{ zuul.branch }} is a variable the jobs have access to, that will have the branch the PR is opened against.

https://zuul-ci.org/docs/zuul/reference/jobs.html#var-zuul.branch

[quidame]

Both are basically the same command:

no, as shown right above.

As for default branch, we don't want this as PRs can be raised against non default branches. {{ zuul.branch }} is a variable the jobs have access to, that will have the branch the PR is opened against.

already done here: https://github.com/ansible/project-config/blob/7ce1629e73bc511fba45dfb2843ef7d5752c5d91/playbooks/ansible-changelog-fragment/run.yaml