CFE-4659: Adds changelog-generator to cfengine-cli#137
Conversation
SimonThalvorsen
force-pushed
the
CFE-4659
branch
2 times, most recently
from
76266ae to
2272521
Compare
Based on the previous changelog-scripts: https://github.com/cfengine/cf-bottom/blob/master/tom/changelog.py and https://github.com/cfengine/core/blob/master/misc/changelog-generator/changelog-generator Made with the intention to easily integrate with gh-action workflow in the future
larsewi
left a comment
larsewi
left a comment
There was a problem hiding this comment.
GJ 🚀 Some smaller comments
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Fetch and merge depndacy upgrades / reverts |
There was a problem hiding this comment.
| # Fetch and merge depndacy upgrades / reverts | |
| # Fetch and merge dependency upgrades / reverts |
| print( | ||
| f"Warning: repo path not found, skipping: {repo_path}", file=sys.stderr | ||
| ) |
There was a problem hiding this comment.
Consider using the logging module. Also, should we perhaps fail here? This way we don't end up with half done changelogs?
| ) | ||
| continue | ||
|
|
||
| os.chdir(repo_path) |
There was a problem hiding this comment.
Instead of changing directory, you can consider using the git -C /path/to/workdir ... option
| proc = subprocess.Popen( | ||
| ["git", "log", "--no-merges", "--reverse", "--pretty=format:%s"] + git_args, | ||
| stdout=subprocess.PIPE, | ||
| ) |
There was a problem hiding this comment.
Is there a particular reason why you are using subprocess.Popen() over the more high-level subprocess.run()?
|
|
||
| m = re.match(REVERT_RE, subject) | ||
| if m: | ||
| dep, frm, to = m.group(1), m.group(2), m.group(3) |
There was a problem hiding this comment.
Avoid abbreviations
| dep, frm, to = m.group(1), m.group(2), m.group(3) | |
| dep, from, to = m.group(1), m.group(2), m.group(3) |
| repo_path = os.path.join(original_dir, repo) | ||
| if not os.path.isdir(repo_path): | ||
| print( | ||
| f"Warning: repo path not found, skipping: {repo_path}", file=sys.stderr | ||
| ) | ||
| continue |
There was a problem hiding this comment.
This code is repeated from above. Maybe it deserves a function to make it more DRY?
| elif re.match(r"^Commit[ .]*$", subject, re.IGNORECASE): | ||
| log_entry_commit = True |
There was a problem hiding this comment.
I've also seen people use Changelog: body. Maybe we should add it as alias to commit?
| # --------------------------------------------------------------------------- | ||
| # Git-log parser | ||
| # --------------------------------------------------------------------------- | ||
| def parse_git_log(repos, git_args): |
There was a problem hiding this comment.
This function is huge. Maybe it would make sense to split it up a bit?
| # --------------------------------------------------------------------------- | ||
| # Repostuff | ||
| # --------------------------------------------------------------------------- | ||
| REPO_SETS = { |
There was a problem hiding this comment.
This is a dictionary of lists, not a set
3 participants
No description provided.