nils-braun / b2luigi

Task scheduling and batch running for basf2 jobs made simple
GNU General Public License v3.0
17 stars 11 forks source link

Fix the proxy creation error #172

Closed Bilokin closed 2 years ago

Bilokin commented 2 years ago

Hello @meliache,

here is a method of fixing the proxy creation error in b2luigi's gbasf2 process. The changes in gb2_proxy_init caused a ioctl error in b2luigi, which can be avoided if we switch to stdin input mode and provide a certificate password explicitly.

Bilokin commented 2 years ago

Thanks. What to do with the failing tests? Adding @mock.patch("getpass.getpass") didn't help

meliache commented 2 years ago

We should add an entry for this change to the CHANGELOG.md under the [Unreleased] section with under a ### Fixed heading (see examples below).

Note for future PR's

Please, always do pull requests from a branch which is not called main. Naming the branch main can be problematic for both you and me. First, if something changes on main before this is merged, your local main and the remote main branches will diverge. It's good practice to have the local main branch follow the remote main branch. Secondly, if this is an editable PR and I can push commits to this PR, this means that I would be able to do changes on your personal main branch. Third, this makes it difficult for me to inspect and test your code locally. Usually, I would inspect the PR locally with

git remote add bilokin https://github.com/Bilokin/b2luigi.git
git fetch --all
git checkout <your-new-branch-name>

If the branch <your-new-branch-name> then only exists on the remote bilokin, git would automatically choose the correct remote for that branch and then create a local branch with the same name tracking the remote branch. Now I had to do

git checkout -b bilokins-main bilokin/main

to checkout your branch locally, which is something I had to look up and didn't know from my head.

Bilokin commented 2 years ago

Sure, I am not use to the github PR methods, so I ended up to use the main branch. I can reopen the PR in a different branch

meliache commented 2 years ago

Sure, I am not use to the github PR methods, so I ended up to use the main branch. I can reopen the PR in a different branch It's fine for now if you don't mind, just as a tip for the future.

Thanks. What to do with the failing tests? Adding @mock.patch("getpass.getpass") didn't help

What to do is to fix the tests or fix the reason why they fail. You can run the tests locally with python -m unittest in the top-directory of the repo. It's some time ago that I wrote them, but if I remember correctly they just change hack the run_with_gbasf function (via mock.patch) to either return a CalledProcessError or to return an output string which contains an error message. I then use the tests to make sure that the code recognizes an error message in the output and handles as expected. The unit test is useful as a definition of how to handle errors. If the gb2_proxy_init would always return a non-zero error code during an error, then I wouldn't need that output parsing code, but I don't know if this is the case by now. Currently I'm afraid I don't have the time to fix the tests, so I would be glad if you could take a look at that.

meliache commented 2 years ago

Thanks for fixing the tests 🙏! Can you just test that the latest version works also in practice? E.g. open a python console (e.g. via basf2 or ipython) and run

>>> from b2luigi.batch.processes.gbasf2 import setup_dirac_proxy
>>> setup_dirac_proxy()

and try both correct and incorrect passwords. I just also added some code to the PR like the try...finally and just would like to make sure that it works.

I wanted to try the above code myself, but it turns out that I have some issue with grid certificate. I renewed it recently but never used it since and seems like I didn't create my .globus files correctly 🙈, it doesn't even work in the terminal.

Update: I now fixed my certificates in the .globus directory and tested with the code snippet above and works perfectly!