-
Notifications
You must be signed in to change notification settings - Fork 10
Add CiFile - Parser and convertor for streamlined CI pipeline/job config file #61
Conversation
BTW, I have made many comments so this is understandable for everybody. Take a look at that too - it basically says most of PR message too :) |
Thanks a lot for your work @gsantner. This will take some time to check out and review. I hope to have that time next week or the week after! |
ehm, is it awaitable to get this merged and worked on? took quite much time to get it in that working state :D |
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've applied the changes that I made comments about. Things that I noticed (list may get longer)
- The
except: [<branch>]
doesn't seem to be evaluated yet (howeveronly: [<branch>]
is) - I think maybe the variables in the
artifact:name
value should be rendered by the CIFile already when usingconvert_to_matrix()
so that user of that matrix doesn't have to
flux/cifile.py
Outdated
# - jobs: Dict of jobs grouped by stage, orded by order in `stages` | ||
# - stages: Name and order of all stages | ||
@classmethod | ||
def convert_to_matrix(self): |
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 doesn't look like this method should be a classmethod
?
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 takes the own internal state
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'm not quite sure I understand what you are saying, sorry. convert_to_matrix()
does indeed take the internal state of self
, specifically, to produce the matrix from self._cifile
. Hower self
is supposed to be an instance of the CiFile
class, and not the class itself, that is why the @classmethod
decorator is inappropriate.
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.
ah, I see, didn't understand before :D you're right
flux/cifile.py
Outdated
|
||
# Print given matrix (from `convert_to_matrix`) to command line | ||
@classmethod | ||
def print_matrix(self, matrix): |
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.
Same as above and for others below.
flux/cifile.py
Outdated
|
||
## Don't move between areas unless knowing consequences | ||
|
||
## Work area 10 |
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.
What's the work area thing?
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.
Don't move between areas unless knowing consequences
it's about this
flux/cifile.py
Outdated
# pip deps: dateparser | ||
import os, sys, threading | ||
import yaml, re | ||
import dateparser |
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 module seems to be unused.
flux/cifile.py
Outdated
stages.append(stage) | ||
|
||
# Add default stage when nothing found | ||
stages=stages if stages else [preference_siai_default_stage] |
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.
preference_siai_default_stage
is not defined anywhere.
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.
Should probably be self._default_stage
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.
yep
flux/cifile.py
Outdated
@classmethod | ||
def _parse_cidict(self, cidict): | ||
# Store dictionary as member | ||
if not cidict or not isinstance(cidict, dict): |
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 doesn't look right -- it should raise an exception if the value passed is invalid rather than just ignoring the fact and using an empty dictionary instead.
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.
doable
With the exmaple CI file that you linked (https://gitlab.com/gsantner/kimai-android/raw/master/.gitlab-ci.yml) on branch
Just so I understand this right, we concatenate the jobs in every stage with every other job to get a single pipeline? So the above would give you two jobs running in Flux CI, running the following Stage's Jobs
If we had another Job in the
? |
We put the job infomration all in same level and add global information (like CI_JOB_ID submitted from Flux CI) to all jobs. We don't put one job into another.
This all above is the matrix view of the data, at the core its just an array of jobs with all having same properties. That was the main point of this CiFile work, that all is streamlined.
No it's will be stage called
No it would be
happy if I can tell more if something unclear |
I understand that they are special because they are actually executed around any other job, and not independent jobs. But then I'm not sure why they are put into their own stages and jobs -- wouldn't it make more sense to put them into a Job directly, as in
The
Maybe I also misunderstand the way you were planning on accessing and executing the before and after scripts per job. |
Yes! But other CIs usually have this special two cases, and we are doing it cleverer by just converting it to be like a normal job :D. (As in, to make existing cifiles easy reusable, without big modifications) Why own stages? Because this > Currently there would be one special case, were you really manually write
I don't quite understand this one. One job has as much
How would we know then that bash,rm belongs to start and mail to the very end of all jobs? Maybe sorry, didn't describe that: one |
This only makes sense to me if you assume that all stages are executed in the same folder and without resetting or flushing the folder between jobs. Does that apply?
Well we know it from the original CI input file -- the global Once we have a full description of the commands for a job, it is no longer important whether a command was part of the |
Exactly. When a job starts, the pwd should be the git root folder, when calling At least till jobs etc would work, thats exactly what I thougt. When we have script/jobs/stages/pipeline working, we could then go on for respecting the
Aah, I think we are talking both about something differently! :D Oh I see actually GitLab CI seems todo that differently or at least I thought so. In GitLab CI it works like you said, with putting all lines before and after job script (didn't use that recently :D). Sorry! Forget what I said before about before/after script! :D I see now. We do want it the way of before/after script of all jobs instead of "before/after pipeline" (thats currently) right? Can change that |
All jobs should be run independently from each other with no leftovers from a previous job (so basically a fresh checkout from the repository). That is why I thought it was a good idea to have the before_script and after_script prepended/appended to the actual job's script. And that is also why it didn't make sense to me to put the before_script and after_script into their own stages.
I think sharing between jobs is an interesting feature, but is that actually a feature in the GitLab pipelines? I can't seem to find info about that anywhere. Artifacts are usually what you can download from the CI after the Job has run. |
Closing as it looks like the project doesn't really goes on. Even though that one is mostly complete, there is still stuff left todo to actually use it into the whole. Which I assume will not happen |
This adds the base for resolving issues #24 #50 and #55.
It sports the quite often mentioned Ci-File parser I have working on many nights.
I have tested it with simple and complex Ci-Files in GitLab CI format, which all is too FOSS, open spec and one of the most powerfullies available currently.
The goal of this PR is not to integrate it into Flux CI already, this is a bigger task, where I'm out of in depth info of Flux CI.
You can see an example on the very bottom, and test things out (too included in file as
__main__
).This basically is what we have todo:
Overall Preperation step:
read_cifile()
Run preperation:
prepare_run_with(
and pass in information from CI itself like pipeline ID, branch, etc. This step is fully about CI and repository data, no user values here.Run:
convert_to_matrix()
jobs:
element in matrix, with jobs grouped by stage. each jobs is ensured to always have ascript:
list of string.One big note: The CiFile logic already does implement job conditional filtering internal, this is something that doesn't need to be implemented separately.
--
License Note: I have this class over at https://github.com/gsantner/opoc/blob/master/python/services/cifile.py, but herebly grant the project to use it freely/fully re-licensed under the Flux projects MIT license.