[Asterisk-code-review] build: Improve handling of CHANGES and UPGRADE.txt for releases. (...repotools[master])
Benjamin Keith Ford
asteriskteam at digium.com
Thu Mar 28 09:57:48 CDT 2019
Benjamin Keith Ford has posted comments on this change. ( https://gerrit.asterisk.org/c/repotools/+/10941 )
Change subject: build: Improve handling of CHANGES and UPGRADE.txt for releases.
......................................................................
Patch Set 5:
(4 comments)
https://gerrit.asterisk.org/#/c/10941/5/process-staging-changes
File process-staging-changes:
https://gerrit.asterisk.org/#/c/10941/5/process-staging-changes@18
PS5, Line 18: sce = StagingChangesExtractor(ast_path=ns.local_root, start_version=ns.start_version, end_version=ns.end_version)
: ret = sce.run()
:
: # Only add trailing slash for printing message; it's not needed otherwise
: if not ns.local_root.endswith("/"):
: ns.local_root += "/"
:
: if ret is 0:
: used_l_opt = ""
: if ns.local_root != "/tmp/":
: used_l_opt = "-l {0} ".format(ns.local_root)
: print("Done! Check {0}asterisk and make sure everything looks right. "
: "Then run 'commit-staging-changes {1}-v {2}'.".format(ns.local_root, used_l_opt, ns.end_version))
: else:
: print("Error! Check the logs and remember to clean up {0}asterisk".format(ns.local_root))
> Could this be extracted into it's own function and reused by mkreleasse. […]
I think the only part that could be reused is instantiating the object, running it, and checking the return value. The rest of it is only useful for the script, imo. Could be an option if combining the commit part of the other script as an option to this.
https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py
File staging_changes.py:
https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py@52
PS5, Line 52: def __init__(self, ast_path, start_version, end_version):
> a "repo" is not passed in, but just the path. […]
Passing in a repo could work (or rather, taking the path and checking to see if the repo exists or not). I'm not sure on checking out the repo though, since the intention of this script is to do work on an existing repository. I would lean towards continuing or exiting the script based on whether or not the repository exists at the path. This also fits with how mkrelease.py does its logic, leaving the repository checkout to it to get stuff ready for the script. Just my opinion, feel free to push back if you feel strongly about it.
https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py@120
PS5, Line 120: with open(staging_path + "/" + filename, 'r') as f:
: subject = ""
: message = ""
: master = False
: line = f.readline()
: while line != "":
: # Get the subject
: if line.startswith("Subject:"):
: if subject != "":
: # Since this is not the first subject, we have data to add
: container = self.Container(subject, message, timestamp)
: if master is True:
: self.master_data.append(container)
: else:
: self.data[subject].append(container)
: message = ""
: subject = line.split("Subject:")[1].lstrip().rstrip()
: line = f.readline()
: if line.startswith("Master-Only:"):
: val = line.split("Master-Only:")[1].lstrip().rstrip()
: if "true" in val or "True" in val:
: master = True
: f.readline()
: else:
: master = False
: line = f.readline()
: message = " * " + line
: elif line == "\n":
: # This is a special case because if there is an empty line separating some
: # text, we don't want to add 3 spaces and then a new line.
: message += "\n"
: else:
: message += " " + line
: line = f.readline()
: container = self.Container(subject, message, timestamp)
: if master is True:
: self.master_data[subject].append(container)
: else:
: self.data[subject].append(container)
: # Clean up the staging directory as we cycle through the files
: LOGGER.debug("Removing file {0}".format(filename))
: os.remove(os.path.join(staging_path, filename))
> While this works, it's kinda "brittle". […]
Yeah I agree with this, I was giving it some thought earlier and it's not very easily maintainable in its current state. I'll look at ways to improve this.
https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py@221
PS5, Line 221: def run(self):
> Another suggestion. If you have a repo after running you could go ahead and add and commit the code. […]
This could be an option (ties in to comment on other file), although I am hesitant to put something like that in the same script since an option like that can be missed and then we would have to deal with that, which is a pain. I like having another script (that could also be used for other purposes, if need be) that doesn't have to go through all the checks of running this one again if run the first time as a "prep".
--
To view, visit https://gerrit.asterisk.org/c/repotools/+/10941
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: repotools
Gerrit-Branch: master
Gerrit-Change-Id: I6dc084afedaeecaf36aaec66c3cf6a5a8ed4ef3c
Gerrit-Change-Number: 10941
Gerrit-PatchSet: 5
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 28 Mar 2019 14:57:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190328/e6962d6d/attachment.html>
More information about the asterisk-code-review
mailing list