dmwm / CRABClient

runrange
14 stars 35 forks source link

completions - generate_completions script works in py3 #5315

Closed mapellidario closed 2 months ago

mapellidario commented 2 months ago

fixes #5314

status

tested on my laptop, but i have a small question before merging

solution

This PR serves multiple purposes:

open questions:

comment

I decided to revamp the generate_completion.py script because I did not want to think about adding manually all the new commands introduced with crab recover :)

cmsdmwmbot commented 2 months ago

Jenkins results:

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-CRABClient-PR-test/1057/artifact/artifacts/PullRequestReport.html

belforte commented 2 months ago

thanks Dario. I am not able to review this. I never knew that generate_completion.py existed and have no idea how and when to use it. Can you simply describe how things will work ? Do we need to run it by hand every time we add or remove a command and pipe output to etc/crab-bash-completion.sh ? Do you intend to have GH do this at any PR or tag ?

belforte commented 2 months ago

And for sure I do not understand the code inside those scripts !

mapellidario commented 2 months ago

I added a commit with and improved comment that i hope is clear enough.

So, at the moment I think we can use it instead of manually editing the file, which means run this script "only" when we add a crab client command or change an option in an existing command.

In the future we can integrate this as part of the crab client build process, so that the completions are generated for every new deployment. I do not think the effort is worth at the moment.

cmsdmwmbot commented 2 months ago

Jenkins results:

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-CRABClient-PR-test/1058/artifact/artifacts/PullRequestReport.html

mapellidario commented 2 months ago

I addressed the some comments from pylint (the ones that i could quickly get rid of without a proper and useless refactoring). If everything goes well, I will merge this PR

cmsdmwmbot commented 2 months ago

Jenkins results:

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-CRABClient-PR-test/1059/artifact/artifacts/PullRequestReport.html

mapellidario commented 2 months ago

jenkins looks good to me, i will merge