[Asterisk-code-review] build: Improve handling of CHANGES and UPGRADE.txt for releases. (...repotools[master])

[Kevin Harwell]

Kevin Harwell 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)

Mostly  just a few suggestions. Feel free to ignore. However, -1 as I do feel like moving the parsing to it's own function is worthwhile since we'll need to do validation.

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.py? (might not make sense, but could be worth consolidating code)


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. What if the path exists, but is not a valid Asterisk repo? I'd suggest passing in a repo object, and if 'None' then create and checkout the repo.


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". Meaning it's not conducive to additions and makes it so the key:value pairs are order dependent. I'd suggest removing the ordering dependency as well as breaking most of this functionality out into it's own function. We'll want to use the same code to check if the file is validly formatted at review time, so we don't run into broken files at release time.


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. If needed you pass in a flag for times when you don't want to do that.



-- 
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: Wed, 27 Mar 2019 23:05:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190327/6523c549/attachment.html>