Skip to content

Improve importer.py #10505

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

Merged
merged 13 commits into from
May 13, 2019
Merged

Improve importer.py #10505

merged 13 commits into from
May 13, 2019

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Apr 28, 2019

Description

Improve python logic and fix an issue when multiple cherry-pick messages has been added

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-tools @alzix

Release Notes

@ciarmcom ciarmcom requested review from alzix and a team April 28, 2019 17:00
@ciarmcom
Copy link
Member

@orenc17, thank you for your changes.
@alzix @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -148,10 +153,14 @@ def get_last_cherry_pick_sha(branch):
lines = output.split('\n')
for line in lines:
if 'cherry picked from' in line:
sha = line.split(' ')[-1]
return sha[:-1]
sha = line.split(' ')[-1][:-1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sha = line.split(' ')[-1][:-1]
# take the last line - git cherry-pick can add this line more
# than once if used multiple times
sha = line.split(' ')[-1][:-1]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to regex

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

# Set logging level
logging.basicConfig(level=level)
rel_log = logging.getLogger("Importer")

if (args.repo_path is None) or (args.config_file is None):
rel_log.error("Repository path and config file required as input. Use \"--help\" for more info.")
exit(1)
rel_log.error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be replaced by required=True in parser

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


repo = abspath(args.repo_path)
if not os.path.exists(repo):
rel_log.error("%s not found.", args.repo_path)
exit(1)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use parser.error or add an action to validate it in the parser

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


json_file = abspath(args.config_file)
if not os.path.isfile(json_file):
rel_log.error("%s not found.", args.config_file)
exit(1)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use parser.error or add an action to validate it in the parser

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@0xc0170 0xc0170 changed the title Imporve importer.py Improve importer.py Apr 29, 2019
Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean up!

Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@adbridge
Copy link
Contributor

adbridge commented May 7, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented May 7, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented May 10, 2019

CI was restarted, all green now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants