-
Notifications
You must be signed in to change notification settings - Fork 3k
Update PSA tools #10010
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
Update PSA tools #10010
Conversation
CI started |
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.
LGTM
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
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.
Meets criteria, PSA feature delivery. Approved.
Moved to a fix type as this now has a code commit as well |
CI restarted |
tools/psa/release.py
Outdated
'commit', | ||
'-m', commit_message | ||
] | ||
|
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.
Have you tried this on both windows and linux ? We had problems running on linux with this format of command passing to subprocess. Hence we do it this way:
def run_cmd(command, exit_on_failure=False):
""" Run a system command returning a status result
This is just a wrapper for the run_cmd_with_output() function, but
only returns the status of the call.
Args:
command - system command as a list of tokens
exit_on_failure - If True exit the program on failure (default = False)
Returns:
return_code - True/False indicating the success/failure of the command
"""
return_code, _ = run_cmd_with_output(command, exit_on_failure)
return return_code
def run_cmd_with_output(command, exit_on_failure=False):
""" Run a system command returning a status result and any command output
Passes a command to the system and returns a True/False result once the
command has been executed, indicating success/failure. If the command was
successful then the output from the command is returned to the caller.
Commands are passed as a string.
E.g. The command 'git remote -v' would be passed in as "git remote -v"
Args:
command - system command as a string
exit_on_failure - If True exit the program on failure (default = False)
Returns:
return_code - True/False indicating the success/failure of the command
output - The output of the command if it was successful, else empty string
"""
text = '[Exec] ' + command
userlog.debug(text)
returncode = 0
output = ""
try:
output = subprocess.check_output(command, shell=True)
except subprocess.CalledProcessError as e:
text = "The command " + str(command) + "failed with return code: " + str(e.returncode)
userlog.warning(text)
returncode = e.returncode
if exit_on_failure:
sys.exit(1)
return returncode, output
Then called by:
cmd = "git rev-parse HEAD"
return_code, sha = run_cmd_with_output(cmd, exit_on_failure=True)
This method works on both platforms .
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.
Tested on Mac
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.
shell=False is a default setting and considered to be more secure as it prevent shell injection. It should be our choice of preference when calling to subprocess.
It works perfectly on linux. But there are some limitations about it that you should be aware of. E.g. aliases are not honored. Environment variables passed via $ sign are not interpolated, e.t.c.
Oren's script is perfectly valid
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.
Tested on Mac
Was expecting more of a "Tested in multiple OSs"
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.
@cmonr I think it's quite good considering it's my day off
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.
@alzix As we are only ever running internally on the ARM network then shell injection is hardly a major concern. I didn't say his script wasn't valid, I merely pointed out that there can be a number of issues running it that way including compatibility on windows and linux, which were born out by much testing of our own scripts. Providing his script runs fine on all the different OS' (which it seems is the case) then I have no problem with it.
NOTE: https://github.com/ARMmbed/mbed-os-release/pull/82 is dependent on this going in first and being compatible. |
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
CI restarted @adbridge review answered? |
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
started CI on latest fix |
@adbridge Tested on windows and linux as well |
Test run: FAILEDSummary: 1 of 9 test jobs failed Failed test jobs:
|
Can you review latest failures? And fix the latest commit typo 1a47960a2b23774c4af219722a0c5149f6e7d596 (the fist line in the commit msg) |
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
@0xc0170 latest test run succeeded. |
* Replace call with check_call to throw exception on failure * Check if binaries actually been changes before calling git commit * Docstrings for all functions * Small refactor
@0xc0170 @NirSonnenschein Typo fixed |
CI restarted |
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
Description
Add option to git commit for each platform
Pull request type
Reviewers
Release Notes