-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add the new release checker script #88
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Paul Elliott <[email protected]>
d62cc44
to
ba896e3
Compare
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.
First round of reviews with several comments/improvements. I will be following up with a second round for more details on the code flow.
In order to run this you must have GH Cli https://cli.github.com installed and authorised to act as | ||
you on github. | ||
|
||
In order to get a GH Cli auth token, either run 'gh auth' or EXPORT $GITHUB_TOKEN (see |
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 would add a note to say what permissions are required for that token, especially with regards to checking out the restricted repository.
For example for a classic token we need
repo:status
Access commit status
repo_deployment
Access deployment status
public_repo
Access public repositories
repo:invite
Access repository invitations
security_events
And possibly read:audit_log
?
I have not been able to make it work using the new limitted scope tokens.
|
||
Checkout and create local branches for the releases you want to check, then run with eg: | ||
|
||
release-checker.py development-local mbedtls.prev.release mbedtls-lts mbedtls.prev.lts.release |
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.
Minor comment change for usability
mbedtls-release-checker.py -vv development.branch mbedtls.prev.release.tag backport.branch mbedtls.prev.lts.release.tag
The script is operating in hashes, and there should be a clear distinction between the mutable ( local branch hashes ) and immutable such as release tags.
|
||
args = parser.parse_args() | ||
|
||
logger = logging.getLogger("release-checker") |
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.
The whole advantage of using the logging class is to do more advanced stuff than print out to stdout.
We should be configuring it to produce output to file
logging.basicConfig(filename='release_helper.log', encoding='utf-8', level=logging.DEBUG)
Optional arguments that could also be used:
filemode="w"
#or "a".stream=sys.stdout
# This could make the next line obsolete since we set the streamhandler to stdout.
logger = logging.getLogger("release-checker") | ||
|
||
log_handler = logging.StreamHandler(stdout) | ||
if args.veryverbose: |
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.
In the current form the script will produce almost no output unless there are Warning. And since there is no write to output file, it is not very user friendly.
I would propose we have the following options for logging.
-v
Verbose. for verbose logging which will enable the debug-s
Silent. Only for warnings. Not a default but for more advanced users and intergration scripts.- We set the info level as default
|
||
try: | ||
|
||
github_repo = MBEDTLS_RESTRICTED_REPOSITORY if args.restricted else MBEDTLS_MAIN_REPOSITORY |
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.
Why is that an either-or ? I would expect in most releases we will need to parse both main and restricted repo and calling the script two times. would it not be better to be able to cater for all combinations? (main repo only, restricted repo only, main and restricted in an invocation)
Ideally I would like something like that.
repositories = [MBEDTLS_MAIN_REPOSITORY] if args.main_repo else []
repositories += [MBEDTLS_RESTRICTED_REPOSITORY] if args.restricted else []
for github_repo in repositories :
... create discrete release classes
This assumes that we also revisit the arguments so the user can adjust the logic without editting the script.
This should also solve the first item from the todo list.
def main(): | ||
|
||
parser = argparse.ArgumentParser(description='Attempt to automatically detect missing backports/Changelog entries for an MbedTLS release') | ||
parser.add_argument('LTS_range_start', help='Git hash/label of LTS branch range start') |
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 would reword it or make it clear in the help section that this is an immutable ref tag.
Also this could have a default set by git describe --abbrev=0 origin/development
|
||
parser = argparse.ArgumentParser(description='Attempt to automatically detect missing backports/Changelog entries for an MbedTLS release') | ||
parser.add_argument('LTS_range_start', help='Git hash/label of LTS branch range start') | ||
parser.add_argument('LTS_range_end', help='Git hash/label of LTS branch range start') |
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.
argparser is using ordereddict so the order of inclusion of the arguments here determines how the will be assigned when parsed.
For example using the syntax described in the docstring release-checker.py development-local mbedtls.prev.release mbedtls-lts mbedtls.prev.lts.release
and printing the vars(args) I noticed that:
/mbedtls-release-checker.py -vv development-local mbedtls-3.4.1 mbedtls-2.28 v2.28.4
{'LTS_output': None,
'LTS_range_end': 'mbedtls-3.4.1',
'LTS_range_start': 'development-local',
'dev_output': None,
'dev_range_end': 'v2.28.4',
'dev_range_start': 'mbedtls-2.28',
'restricted': False,
'verbose': False,
'veryverbose': True}
Ideally I would adjust the order to be like like that mbedtls.prev.release
- mbedtls.dev.branch
- mbedtls.prev.lts.release
- mbedtls.lts.branch
Also I would use the standard long and short notation for all arguments.
e.g.
def get_latest_tag(lts=False):
""" Return the latest tags for release and lts releases """
cmd = ["git","describe","--abbrev=0"]
cmd += ["origin/mbedtls-2.28"] if lts else ["origin/development"]
out = subprocess.check_output(cmd) # Add try/catch logic
return out.decode('UTF-8').strip()
parser.add_argument("-ltr", "--lts-release", default=get_latest_tag(lts=True), help="Tag pointing to the latest LTS release",`
parser.add_argument("-ltb", "--lts-branch", default="mbedtls-2.28", help="Branch containing all the code to be included in the new release",
parser.add_argument('dev_range_start', help='Git hash/label of development branch range start') | ||
parser.add_argument('dev_range_end', help='Hash/label of development branch range end') | ||
parser.add_argument('-lo', '--LTS_output', help='Ouput file to dump LTS release csv data to') | ||
parser.add_argument('-do', '--dev_output', help='Ouput file to dump development release csv data to') |
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 would have predefined defaults for those files. This script takes a significant ammount and API calls to go through a run, that will do nothing in the current default configuration.
It would make more sense for the user to explicitly turn off output file generation.
This is the first working version of the release checker script, that was used to generate the 3.4.0 / 2.28.3 releases.