Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preserve %autochangelog in downstream spec file #1453

Merged
merged 1 commit into from Jan 21, 2022
Merged

Preserve %autochangelog in downstream spec file #1453

merged 1 commit into from Jan 21, 2022

Conversation

majamassarini
Copy link
Member

@majamassarini majamassarini commented Jan 17, 2022 •
edited

When the downstream spec file has the %autochangelog macro then preserve it in downstream.
Do not preserve it if sync_changelog option is set.

Fixes #1116

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

lachmanfrantisek
lachmanfrantisek reviewed Jan 17, 2022
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I have just one small suggestion and one note about the argument name.

packit/base_git.py Outdated
"""
update this specfile using provided specfile

:param specfile: specfile to get changes from (we update self.specfile)
:param version: version to set in self.specfile
:param comment: new comment for the version in %changelog
:param preserve_autochangelog: preserve the %autochangelog macro
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit confused by the name of the argument at first. From the first usage (if at line 371), it looks like has_autochangelog but the second occurrence (if at line 384) looks more like an add_new_changelog_entry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it seems confusing.
I tried to refactor this but line 370 made me roll back all the changes.
If I understand properly the code, in line 370 the downstream spec file content is overwritten by the original downstream spec file content. And you already told me a couple of good reasons for doing it.

In line 371, I am preserving the upstream %autochangelog macro (if any in the upstream spec file). In the case, as an example, that the original downstream spec file does not have one. I think this could happen, am I wrong?

In line 384, the version and the changelog are updated, if any. You are right, it seems I add a new empty changelog entry because I need to add a new version. I can not split changing the version and changing the changelog. Because any change to the version will be potentially overwritten by line 370 called when changing the changelog... thus the version must be updated only after that the changelog is updated...

I am not sure I answered your concerns. Please let me know If you have any suggestion on how to improve this :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have anything about the implementation, but am just thinking if we can find a better name for that argument..;) But thanks for the detailed comment. Now I see why it's like this.

Maybe just more detailed parameter documentation will be enough. What about mentioning that when preserve_autochangelog is set, we sync the macro to downstream and add no changelog entry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure

packit/utils/changelog_helper.py Outdated Show resolved Hide resolved
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

csomh
csomh reviewed Jan 18, 2022
View reviewed changes
packit/specfile.py Outdated
Comment on lines 43 to 46
for value in self.spec_content.section("%changelog"):
if "%autochangelog" in value:
return True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about checking if there is any line that starts with %autochangelog instead? The current implementation would return True even for a comment line that contains %autochangelog (which would be incorrect).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably go with the safer way:

  • %changelog contains only %autochangelog → True
  • %changelog contains %autochangelog and something else → raise an exception
  • otherwise → False

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably go with the safer way:

* `%changelog` contains only `%autochangelog` → `True`

* `%changelog` contains `%autochangelog` and something else → raise an exception

* otherwise → `False`

I am not raising an exception because I thought that maybe it is too much to raise it if, as an example, there is a comment inside the section.

packit/utils/changelog_helper.py Outdated Show resolved Hide resolved
mfocko
mfocko reviewed Jan 18, 2022
View reviewed changes
Copy link
Member

@mfocko mfocko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good :)

packit/base_git.py Outdated Show resolved Hide resolved
packit/base_git.py Show resolved Hide resolved
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@csomh
Copy link
Contributor

csomh commented Jan 18, 2022

So I just noticed, that the issue asks for preserving %autochangelog if present downstream (that is it's in the spec-file in dist-git).:

propose-downstream should not modify downstream spec %changelog if it uses %autochangelog.

But the current PR is doing something else:

When the upstream spec file has an %autochangelog macro then preserve it in the downstream spec file too.

@jpopelka got confused the same way, as seen in his comment.

Doing what this PR does, that is setting %autochangelog if it's present upstream, can be already achieved by setting sync_changelog to true in packit.yaml, which would overwrite the changelog downstream with the upstream changelog content (which would be set to %autochangelog in this case).

I think, we kinda got lost on this change 😄

But if this is the case, then we should clarify how such a functionality (that is: preserving %autochangelog downstream) would interact with sync_changelog and copy_upstream_release_description.

My suggestion is that when sync_changelog is set to true, the upstream changelog should be copied verbatim to dist-git, regardless if dist-git uses %autochangelog or not. This would enable users to change a downstream %autochangelog to something else. Note, that this already is the current behaviour.

On the other hand if sync_changelog is false (the default), then an %autochangelog in dist-git should be preserved, regardless of how copy_upstream_release_description is set. This PR already implements most of this, just needs some tweaks here and there, for example the preserve_autochangelog flag should be set based on the spec file in the dist-git repo, not based on the one found in the upstream repo. Tests will also need to be updated to reflect the change in functionality.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@majamassarini
Copy link
Member Author

I hope I got the right point of view, this time :)

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

csomh
csomh reviewed Jan 19, 2022
View reviewed changes
packit/specfile.py Outdated Show resolved Hide resolved
packit/specfile.py Outdated Show resolved Hide resolved
packit/specfile.py Outdated Show resolved Hide resolved
packit/specfile.py Outdated Show resolved Hide resolved
packit/utils/changelog_helper.py Outdated Show resolved Hide resolved
packit/utils/changelog_helper.py Show resolved Hide resolved
tests/integration/test_update.py Show resolved Hide resolved
tests/unit/test_specfile.py Outdated Show resolved Hide resolved
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

csomh
csomh reviewed Jan 20, 2022
View reviewed changes
packit/utils/changelog_helper.py Outdated Show resolved Hide resolved
tests/unit/test_specfile.py Outdated Show resolved Hide resolved
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

csomh
csomh approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@csomh csomh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

lachmanfrantisek
lachmanfrantisek approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small code-style suggestion, otherwise it looks really good.

And thanks for the tests!

packit/specfile.py Outdated Show resolved Hide resolved
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

lachmanfrantisek
lachmanfrantisek approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🚀

mfocko
mfocko approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@mfocko mfocko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@majamassarini majamassarini added the mergeit When set, zuul wil gate and merge the PR. label Jan 21, 2022
@majamassarini
Copy link
Member Author

recheck

@majamassarini majamassarini merged commit 7c3b005 into packit:main Jan 21, 2022
@majamassarini majamassarini deleted the fix/1132 branch January 21, 2022 12:27
@mfocko
Copy link
Member

mfocko commented Jan 21, 2022

recheck

@majamassarini recheck runs only the Zuul, if you want to trigger just the Testing Farm (which is failing for el8) you have to post /packit test (and for the sake of enumerating CI commands ;) if you want to run whole packit-service (from SRPM build through Copr to Testing Farm) you can post /packit build, and also on GitHub you can trigger it from the GitHub Check Run, when you click on the details of the failed runs there should be re-run button)

And now for the cause, it seems to me, that there is some older version of dependency that doesn't support %autochangelog in the %changelog section, so rerunning the tests will probably not help. It looks like it's from rebasehelper.

@majamassarini
Copy link
Member Author

I merged the PR manually few seconds before be able to see your comment. I hope it is not a problem. And thanks for explaining me how to trigger all the other builds.

@mfocko
Copy link
Member

mfocko commented Jan 21, 2022

I merged the PR manually few seconds before be able to see your comment. I hope it is not a problem. And thanks for explaining me how to trigger all the other builds.

No problem, Zuul would merge it too, cause the tests ran in Zuul passed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpopelka jpopelka jpopelka left review comments

@mfocko mfocko mfocko approved these changes

@csomh csomh csomh approved these changes

@lachmanfrantisek lachmanfrantisek lachmanfrantisek approved these changes

Assignees
No one assigned
Labels
mergeit When set, zuul wil gate and merge the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

propose-downstream to support %autochangelog
5 participants
@majamassarini @csomh @mfocko @jpopelka @lachmanfrantisek