-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Add configurable options to generate CI #2420
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
Changes from all commits
60fd1a4
d31053e
1e7fdef
aa1cbf3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ var/ | |
| *.egg-info/ | ||
| .installed.cfg | ||
| *.egg | ||
| external/ | ||
|
|
||
| # Ignore Mac DS_Store files | ||
| .DS_Store | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| import os | ||
| import sys | ||
| import copy | ||
| import yaml | ||
|
|
||
| import reframe.core.exceptions as errors | ||
|
|
@@ -21,6 +22,17 @@ def _emit_gitlab_pipeline(testcases, child_pipeline_opts): | |
| recurse = config.get('general/0/check_search_recursive') | ||
| verbosity = 'v' * config.get('general/0/verbose') | ||
|
|
||
| # Collect the generate CI options | ||
| before_script = config.get('generate-ci/0/before_script') | ||
| after_script = config.get('generate-ci/0/after_script') | ||
| artifacts = config.get('generate-ci/0/artifacts') | ||
| artifacts_expiry = config.get('generate-ci/0/artifacts_expiry') | ||
|
|
||
| # Need to append prefix to artifacts | ||
| artifacts = [os.path.join(prefix, a) | ||
| if a in ['perflogs', 'stage', 'output'] | ||
| else a for a in artifacts] | ||
|
|
||
| def rfm_command(testcase): | ||
| # Ignore the first argument, it should be '<builtin>' | ||
| config_opt = ' '.join([f'-C {arg}' for arg in config.sources[1:]]) | ||
|
|
@@ -64,9 +76,13 @@ def rfm_command(testcase): | |
| for tc in testcases: | ||
| json[f'{tc.check.unique_name}'] = { | ||
| 'stage': f'rfm-stage-{tc.level}', | ||
| 'before_script': copy.deepcopy(before_script), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need the deepcopy here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I do not remember. Probably it is vestigial as I was trying out few things back then. Probably we can remove it. |
||
| 'script': [rfm_command(tc)], | ||
| 'after_script': copy.deepcopy(after_script), | ||
| 'artifacts': { | ||
| 'paths': [f'{tc.check.unique_name}-report.json'] | ||
| 'paths': [f'{tc.check.unique_name}-report.json', | ||
| f'{tc.check.unique_name}-report.xml'] + artifacts, | ||
| 'expire_in': artifacts_expiry, | ||
| }, | ||
| 'needs': [t.check.unique_name for t in tc.deps] | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -394,6 +394,32 @@ | |||||||||||||||
| "additionalProperties": false | ||||||||||||||||
| } | ||||||||||||||||
| }, | ||||||||||||||||
| "generate-ci": { | ||||||||||||||||
| "type": "array", | ||||||||||||||||
| "items": { | ||||||||||||||||
| "type": "object", | ||||||||||||||||
| "properties": { | ||||||||||||||||
| "before_script": { | ||||||||||||||||
| "type": "array", | ||||||||||||||||
| "items": {"type": "string"} | ||||||||||||||||
| }, | ||||||||||||||||
| "after_script": { | ||||||||||||||||
| "type": "array", | ||||||||||||||||
| "items": {"type": "string"} | ||||||||||||||||
| }, | ||||||||||||||||
| "artifacts": { | ||||||||||||||||
| "type": "array", | ||||||||||||||||
| "items": { | ||||||||||||||||
| "type": "string", | ||||||||||||||||
| "enum": ["perflogs", "output", "stage"] | ||||||||||||||||
| } | ||||||||||||||||
| }, | ||||||||||||||||
| "target_systems": {"$ref": "#/defs/system_ref"}, | ||||||||||||||||
| "artifacts_expiry": {"type": "string"} | ||||||||||||||||
| }, | ||||||||||||||||
| "additionalProperties": false | ||||||||||||||||
| } | ||||||||||||||||
| }, | ||||||||||||||||
| "logging": { | ||||||||||||||||
| "type": "array", | ||||||||||||||||
| "items": { | ||||||||||||||||
|
|
@@ -506,6 +532,11 @@ | |||||||||||||||
| "required": ["systems", "environments", "logging"], | ||||||||||||||||
| "additionalProperties": false, | ||||||||||||||||
| "defaults": { | ||||||||||||||||
| "generate-ci/after_script": ["echo 'noop'"], | ||||||||||||||||
| "generate-ci/before_script": ["echo 'noop'"], | ||||||||||||||||
| "generate-ci/artifacts": [], | ||||||||||||||||
| "generate-ci/artifacts_expiry": "30 days", | ||||||||||||||||
| "generate-ci/target_systems": ["*"], | ||||||||||||||||
|
Comment on lines
+535
to
+539
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't those too specific to Gitlab? I had also this a bit differently in mind. That we allowed the users to specific any structure, which we could then combine with reframe's generated one. What about something like this?
Suggested change
And the users could write: "ci-integration": [
{
"target_system": ["*"],
"backend": "gitlab",
"pipeline": {
"after_script": ["echo noop"],
"before_script": ["echo noop"],
"artifacts": ["output"],
"artifacts_expiry": "30d"
}
}
]
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree this is too specific for Gitlab. I like your idea and it gives us more freedom to add other backends in the future. I was thinking more along using a template and then merging it with the ReFrame generated one. The rationale is GitLab CI provides ton of variables for CI. How many of them can we actually support using the current approach? Also, we need to write logic to get the YAML file syntax right. I mean placing these keywords in proper hierarchy. With a template approach, we delegate this to the user to provide a template with correct syntax and keywords. For instance, ReFrame produces a child YAML file something like this: A template can be something like this: The contents of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense what you propose here. Is your template a template for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I am proposing here is the full
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In principle, I'm in favor of the idea, but I need to understand a bit the workflow. What is the template that reframe reads and what does it generate exactly? That'd help before starting the implementation I think.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's take the OSU test example to see how this will work. Current ReFrame CI generator If we generate CI child pipeline for this test, we will end up with the following yaml file: Our aim here is to provide arbitrary configurability for this generated yaml file. Proposal Suppose the user wants to configure his/her pipeline with keywords The important thing here is we define template per stage. We know the number of stages that will be generated a priori and naming convention is pretty straight-forward here If the user passed yaml file has syntax errors, the generated file will inherit them too. So it is the responsibility of the user to get the template file right. This way the user can arbitrarily configure the CI pipelines with all the options provided by the Gitlab. This also helps the developers to add support for other backends easily with minimal configuration implemented in ReFrame and delegating the advanced config options to user via templates. I was thinking along these lines and of course we can improve this idea further. Please let me know if the workflow is clear.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @mahendrapaipuri, I finally had some time to put my thoughts together on this. Your idea is in the right direction, but we could tweak it a bit:
stage-template-x:
match:
stage: rfm-stage-[12]
ci-config:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
build-template:
match:
job: OSUBuildTest.*
ci-config:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /outputIn this file you write CI templates and you request them to either match a job (aka test) or a stage. The framework when generating stage X will check if there is a matching template and if it exists, it will merge its What do you think about this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @vkarak, Sorry for late response, I was away. I like your template approach and it is more elegant. I have a question on how do we pass this template:
And we directly match the key of the dict with job name in generated CI. With inline can look like this: How does it sound to you?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you're suggesting to drop the matching for stages and keep only that for tests. I'm fine with that. Also I wouldn't have a problem with your template proposal, assuming that's it's valid yaml syntax to have special characters, such OSUBuildTest.*:
include: /path/to/ci/templates/OSU/OSUBuildTest
OSUAllreduceTest.*:
include: /path/to/ci/templates/OSU/OSUAllreduceTestwill be equivalent to this, assuming the contents of the template files are the same, right? OSUBuildTest.*:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /output
OSUAllreduceTest.*:
before_script:
- source $HOME/.bashrc
artifacts:
paths:
- rfm-stage/${CI_COMMIT_SHORT_SHA} /outputI'm fine with this proposal. |
||||||||||||||||
| "environments/modules": [], | ||||||||||||||||
| "environments/env_vars": [], | ||||||||||||||||
| "environments/variables": [], | ||||||||||||||||
|
|
||||||||||||||||
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 probably support placeholders in the configuration, so that you can write:
{ 'artifacts': ['{CI_PREFIX}/foo.log', ...] }And here you do something like this:
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.
Sounds good for me. Would make this change! Cheers!!