-
Notifications
You must be signed in to change notification settings - Fork 15
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
[irods/irods#7229] Add support for iRODS 5 #231
Conversation
e2c0b4f
to
44cdd5d
Compare
I've confirmed the changes in this PR work. There was one thing that caught my eye. When using -vvv, I noticed a strange message which seemed to imply the IrodsController replacement for irodsctl wasn't working. This was observed in the output during setup. I'm not entirely sure it was real. The results of the tests appear to be correct. I believe it was during federation testing. |
The following is from a unit test run for iRODS 4.3.3 (using this PR) and is exactly the message I was referring to. The test passed BTW.
Notice the last line from the logs above. It is produced by the following: irods_testing_environment/irods_testing_environment/irods_setup.py Lines 439 to 449 in 615a2e2
I then did the same thing again, but using tip-of-main for the testing environment and got this output.
Notice the use of Based on the output of each run, I'm sure this is expected/intentional. Therefore, everything is working correctly. |
but i don't see in the log ... |
That log message appears after those lines. |
oh, okay - so we're failing to stop(), and that's 'okay'? |
That code snippet is designed to cover multiple situations based on the state of the world. Based on what I saw for tip-of-main for this repo, I think we're okay. |
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.
Looks good overall. Just some suggested improvements.
do we have a linter we can run? something that will clean this kind of thing up? |
Created #233 for linter workflow. I also kinda addressed all the problems it found in this branch: https://github.com/alanking/irods_testing_environment/tree/ruffaction Not sure if we actually want those changes but there they are. |
I say open a PR for that. |
Okay, I opened the PR here: #234 Left it in draft in case we wanted to put this in first. |
Yes, I'd like to get this PR merged first unless you feel it's easier to merge the one you just opened. The edits in this PR aren't difficult to apply on top of yours. Thoughts? |
No preference. I don't think the merge conflict will be too bad, so I don't mind dealing with it in the other PR. In other words, let's put this one in first. |
agreed - linter/formatter after please. |
Attached issue numbers to commits. Just one more comment to resolve before this is ready. |
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.
If there's no further reservations about the ODBC drivers, I say # it.
Still thinking about the ODBC driver issue. I'm going to back that change out so we can investigate it more. The other changes are fine. |
Don't merge this yet. Got the wrong issue number for irods/irods. |
…ion of federation initialization. Changes to config files in iRODS 5 are not automatically loaded, therefore, the testing environment must instruct the iRODS 5 server to reload its config files before launching the tests.
…on for containers from 64mb to 100mb. The tests for iRODS 5 lean heavily on configuration reloading. This operation results in at least two agent factories appearing, increasing the amount of shared memory needed by the container. This commit makes it so that the tests don't terminate due to a server reload.
… irodsctl. irodsctl was removed in iRODS 5. This commit replaces the testing environment's use of irodsctl with the python code invoked via irodsctl.
… for setup_irods.py.
Removed the ODBC driver commit and fixed issue numbers. Ready for merging. |
Related to irods/irods#7988.
This is a WIP.The change to the shared memory is required because iRODS 5 temporarily allocates more shared memory during a reload of the configuration.
Feel free to ask questions and provide feedback.