fix checking for new easyconfigs in copy_easyconfigs, pick up specified commit message as PR title if none was provided #2259
Conversation
…ng (and negate to fix value)...
…orcing to provide both
|
Some broken tests went unnoticed because not all the tests for the GitHub integration functionality are run for pull requests, due to the lack of an available GitHub token (it's not provided for PRs to avoid it gets leaked via a rogue PR). |
easybuild/tools/github.py
Outdated
| main_title = ', '.join(names_and_versions[:3] + ['...']) | ||
|
|
||
| title = "{%s}[%s] %s" % (class_label, toolchain_label, main_title) | ||
| elif commit_msg: |
There was a problem hiding this comment.
Are file_info['ecs'] and all(file_info['new']) and not deleted_paths and commit_msg mutually exclusive? I guess they aren't, but if they are, I'd suggest to swap the order of the ifs, since evaluating file_info['ecs'] and all(file_info['new']) and not deleted_paths is way more expensive than commit_msg. Nitpicking, I know.
There was a problem hiding this comment.
Actually, you may have a good point there, although efficiency isn't a real concern here.
If a custom commit message is specified, that should determine the PR title, rather than an auto-guessed title.
I'll flip them around, thanks for the suggestion!
|
@damianam all good now? |
test/framework/easyconfig.py
Outdated
| ]) | ||
| write_file(toy_ec, toy_ec_txt) | ||
|
|
||
| # verify whether copied easyconfig gets cleaned up |
There was a problem hiding this comment.
Is this comment accurate? I don't think so
There was a problem hiding this comment.
it sort of is, but the actual check is deeper down...
test/framework/easyconfig.py
Outdated
|
|
||
| self.assertEqual([len(x) for x in res.values()], [1, 1, 1]) | ||
|
|
||
| self.assertTrue(isinstance(res['ecs'][0], EasyConfig)) |
There was a problem hiding this comment.
This is checked for in line 1815 already. Why do the same kind of check again?
|
lgtm now. Going in after travis is done. |
2 participants
No description provided.