ceph-dev-pipeline: extract matrix stage script bodies into helper funcs#2621
ceph-dev-pipeline: extract matrix stage script bodies into helper funcs#2621shraddhaag wants to merge 1 commit into
Conversation
|
I would like to see very obvious comments above each function. Additionally, I think it'd be beneficial for each function to have a number preceding it. Something like, etc. |
|
This worked as a concept: https://jenkins.ceph.com/job/preserve-ceph-dev-pipeline-dgalloway/37/ |
978e88f to
58f31e1
Compare
done!
I think function names starting with numbers is not allowed by most languages, including Groovy. I can perhaps do something like If not, could you please help me understand why we want numeric to start the function name? |
Can you make them more obvious? Include a stage number.
Ah, right, fair enough. Good point. Skip it. I think "StageN" is worse. Loud comments should be enough. |
|
We have a number of very large changes incoming to ceph-dev-pipeline so I would ask that we hold off on merging this until those are sorted. Then we can split the new stages back out. |
|
I very much like the idea behind this PR! One change I would like to see, for readability / maintainability / safety reasons is the new functions taking arguments instead of depending on environment variables to be set. |
4895d71 to
bedaaf1
Compare
@djgalloway I saw that #2623 was merged which made substantial changes to ceph-dev-pipeline. I've rebased against main and extracted the new added logic into its own contained helper function. If we could review + test this PR one last time that will be great. I am blocked on this to enable debug builds for noble (#2622).
@zmc Thanks Zack! I definitely like the idea of explicit argument declaration. I went ahead and implemented it but turns out it is quite a major change touching code which seems out of scope of this PR. I've created a new PR with the respective changes: #2635 which we can test + merge once this one is in. If you'd prefer for both commit to land together in this PR itself, please let me know. |
Problem: We were hitting JVM's 64KB bytecode limit when adding new distros to the matrix stage script. Solution: Add helper functions for each stage in the matrix stage script, such that the original block is significantly reduced, thus ensuring we do not hit the JVM's 64KB limit again. This will also ensure we can keep extending the matrix with more distros without worrying about this issue. Signed-off-by: Shraddha Agrawal <shraddha.agrawal000@gmail.com>
bedaaf1 to
dd8f6ce
Compare
Description
Problem: We were hitting JVM's 64KB bytecode limit when adding new distros to the matrix stage script. Example: #2612 (comment).
Solution: Add helper functions for each stage in the matrix stage script, such that the original block is significantly reduced, thus ensuring we do not hit the JVM's 64KB limit again. This will also ensure we can keep extending the matrix with more distros without worrying about this issue.
Testing
I have been able to successfully parse the XML using:
jenkins-jobs --conf ~/.jenkins_jobs.ini test ceph-dev-pipeline/config/definitions/ceph-dev-pipeline.yml. I am unable to actually test with this as I do not have permissions to update the pipelines:shraddhaag is missing the Job/Configure permission. If someone can help test this out, that would be great!