-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Add support to generate dynamic Gitlab pipelines #1572
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
Conversation
|
Hello @victorusu, Thank you for submitting the Pull Request!
Do see the ReFrame Coding Style Guide |
vkarak
left a comment
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 see two parts of this PR: (a) selecting the tests for which to generate a pipeline and (b) the generation of the pipeline. Currently these parts are mixed up and they should be separated. Part (a) must be reconsidered completely, since it will be obsolete as soon as #1594 is merged. The second one should be independent: generate a pipeline file given a set of test cases. This would be also testable, whereas it's not as it is written now.
Finally, I don't understand how we plan the --ci-tags and --ci-generate to interoperate with the rest of the test selection options.
| return filepatt.format(sessionid=new_id) | ||
|
|
||
|
|
||
| def filter_checks(checks_found, options, rt, loader, ci_tag=None): |
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.
This will need to immense conflicts when #1594 is merged. I would suggest to revert its original location.
| # An alternative design would be to have environment variables exported | ||
| # during the trigger phase of reframe that would influence the tests | ||
| # individually | ||
| def generate_ci_pipeline_file(ci_pipeline_file, ci_tags, checks_found, options, rt, loader, site_config): |
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.
All the CI integration functionality must go into a separate module.
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.
And the interface seems a bit hacky, too.
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 reason is because this function mixes everything: selection of tests and the generation of the pipeline. These must be separated and, especially, the selection of the tests must be viewed in the light of the work done in #1594.
| # + --module-mappings | ||
| # + --failure-stats | ||
| # + --performance-report | ||
| for test in checks_matched: |
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.
Everything must happen at the test case level. Now the test cases are generated immediately and everything in the frontend operates on them.
| # - about the configuration file to use: reframe's -C option | ||
| # - about the artifacts artifacts: {paths: [jobs_scratch_dir], when: always} | ||
|
|
||
| # TODO there are some missing options: |
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.
It's not clear to me how you want to treat the different flags. Can you explain?
|
I am closing this PR, because another one (#1641) replaces it. |
Fixes #1523.