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.
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
Changelog refactor #1367
Changelog refactor #1367
Conversation
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely factored out! would be great to have more sensible method names with proper docstrings
packit/utils/changelog_helper.py
Outdated
release=release_to_update, changelog_entry=msg | ||
) | ||
|
||
def prepare_upstream_locally(self, version: str, commit: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename this method to something which would resemble what it actually does; update_spec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current name is the caller…
self.dg.get_absolute_specfile_path(), | ||
) | ||
|
||
def prepare_upstream_using_source_git(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dtto
@mfocko a friendly reminder you have this one still opened :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice clean up! I agree with Tomáš about the names and docstrings. Otherwise LGTM and ready to go!
(Can you please document the new action in packit.dev
?)
Thanks!
dda6bd0
to
1c1302d
Compare
Build succeeded.
|
if not messages: | ||
return None | ||
|
||
return "\n".join(map(lambda line: line.rstrip(), messages)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely done wrt .rstrip()
, since the entries may contain important leading whitespace
Returns: | ||
Changelog entry or `None` if action is not present. | ||
""" | ||
messages = self.up.get_output_from_action(ActionName.changelog_entry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it makes sense to provide some env vars here to assist with the generation:
- spec file? (well, they know where it is and it's constant)
- project name, package name? dtto
- version name of the release in-progress - this one would make sense even though HEAD should be git-tagged, so also easy to find out
- git-log data - easy to find out
- previous release - we don't know that
- what else?
1c1302d
to
c8a4504
Compare
Build succeeded.
|
Signed-off-by: Matej Focko <mfocko@redhat.com>
Signed-off-by: Matej Focko <mfocko@redhat.com>
Signed-off-by: Matej Focko <mfocko@redhat.com>
Signed-off-by: Matej Focko <mfocko@redhat.com>
Signed-off-by: Matej Focko <mfocko@redhat.com>
Signed-off-by: Matej Focko <mfocko@redhat.com>
c8a4504
to
159c4c7
Compare
Build succeeded.
|
Build succeeded (gate pipeline).
|
TODO:
Fixes #1299
Related to
Merge before/after
Packit supports
changelog-entry
action that is used when creating SRPM, the action is supposed to generate whole changelog entry (including-
at the start of the lines) and has a priority over any other way we modify the changelog with.