Atomic avu handling (main) #65

FifthPotato · 2024-01-09T09:55:48Z

Primary code mostly appears to work as expected, but can't get the test to stick. Calling it through the microservice seems to bypass policy (at least, I think) and I'm not sure if we can use the python client in tests. At a bit of a loss on how to properly test it with just icommands...

trel · 2024-01-09T14:30:27Z

fair. i don't think icommands have a binary that hits this API endpoint. we expose it via PRC... and yes, the microservice is already 'in the server', so wouldn't hit a PEP.

trel · 2024-01-09T14:35:29Z

and perhaps a run through clang-format? i see some long lines.

korydraughn · 2024-01-09T14:44:14Z

We're going to have to provide another mechanism for testing the atomic PEP. Creating a dependency on the PRC seems like the easiest path forward.

trel · 2024-01-09T14:54:22Z

agreed, for now.

CMakeLists.txt

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

alanking · 2024-01-16T18:34:34Z

Is this in draft because we are waiting on something? Please mark as ready for review when ready. Thanks

korydraughn

Do the tests pass? What's left for this?

src/main.cpp

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

alanking

Most of my comments are related to code readability. Approach seems sound.

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

packaging/test_rule_engine_plugin_metadata_guard.py

src/main.cpp

korydraughn

Found a few more things.

src/main.cpp

irods_consortium_continuous_integration_test_hook.py

alanking

Getting there...

src/main.cpp

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

trel · 2024-01-25T13:04:49Z

please mark conversations as resolved/done when they are resolved/done. it's so close...

alanking

Lookin real nice, yep. Noticed one thing that I missed before, again, if I'm reading things properly. :)

src/main.cpp

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

alanking

I think we are ready for some squashing, no #s yet.

korydraughn

Looking real good. Found a few more things.

src/main.cpp

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

korydraughn

This is looking really good.

src/main.cpp

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

src/main.cpp

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py

FifthPotato · 2024-01-31T19:00:58Z

I think I got it all. Did I miss anything?

korydraughn · 2024-01-31T19:21:24Z

Have you run all the plugin tests (i.e. the new ones and the old ones) to see if everything is passing?

If not, please do so and post a comment letting us know if they passed or failed. We'll do one more pass over the PR while that's happening.

korydraughn

I think we're in a good spot.

Will wait for test results before approving.

Good job!

FifthPotato · 2024-01-31T19:35:34Z

Yep, I've run them, and both old&new pass.
This will also potentially require PRC to be added to the testing environment dockerfiles, methinks.

alanking · 2024-01-31T19:41:06Z

This will also potentially require PRC to be added to the testing environment dockerfiles, methinks.

Please create an issue for this and maybe link to this PR: https://github.com/irods/irods_testing_environment Thanks!

korydraughn · 2024-01-31T19:50:38Z

Can the test hook install the PRC?
Is it weird to install python modules from a python script?

@alanking Who launches the test hook in the testing environment? root or irods?

alanking · 2024-01-31T19:53:49Z

Yeah, root runs the test hook in the testing environment now. A user with sudo access should run the test hook as they frequently install packages using the package manager and configure/run external services.

We install Minio packages via pip in the test hook for the S3 resource plugin. Not sure if it's weird, but there is precedent.

korydraughn · 2024-01-31T19:57:32Z

Good.

@FifthPotato Let's start with making the test hook install the PRC. I imagine it shouldn't take no more than one or two lines of code.

irods_consortium_continuous_integration_test_hook.py

korydraughn · 2024-02-01T17:18:03Z

You could include issue 42 as well since it's marked as a duplicate of 38?

korydraughn · 2024-02-01T17:22:06Z

Please add an empty line between the main commit message and the lines that act as the body. For example:

[38,42] Primary commit message / Summary.

Body.

FifthPotato · 2024-02-01T17:26:00Z

Okay, messed with it until the commit msg doesn't get cut off, and put the rest into the body. Numbers added, too.

korydraughn

Excellent! Pound it.

No spaces after the comma in the square brackets (i.e. [#38,#42]).

metadata_guard now properly accounts for atomic metadata operations. Added unit tests, which use PRC.

FifthPotato · 2024-02-01T17:39:00Z

Octothorpe'd

trel reviewed Jan 16, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

trel reviewed Jan 16, 2024

View reviewed changes

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

trel reviewed Jan 16, 2024

View reviewed changes

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

korydraughn reviewed Jan 17, 2024

View reviewed changes

FifthPotato marked this pull request as ready for review January 19, 2024 14:29

alanking reviewed Jan 19, 2024

View reviewed changes

korydraughn reviewed Jan 23, 2024

View reviewed changes

alanking reviewed Jan 23, 2024

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

src/main.cpp Show resolved Hide resolved

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Show resolved Hide resolved

alanking reviewed Jan 25, 2024

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

alanking referenced this pull request Jan 25, 2024

Still review changes...

dcdab03

alanking reviewed Jan 26, 2024

View reviewed changes

alanking changed the title ~~Atomic avu handling~~ Atomic avu handling (main) Jan 26, 2024

korydraughn reviewed Jan 26, 2024

View reviewed changes

korydraughn reviewed Jan 29, 2024

View reviewed changes

korydraughn reviewed Jan 30, 2024

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

korydraughn reviewed Jan 31, 2024

View reviewed changes

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Show resolved Hide resolved

trel reviewed Jan 31, 2024

View reviewed changes

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

packaging/irods_prc_tests/test_rule_engine_plugin_metadata_guard_atomic.py Outdated Show resolved Hide resolved

korydraughn reviewed Jan 31, 2024

View reviewed changes

alanking reviewed Feb 1, 2024

View reviewed changes

irods_consortium_continuous_integration_test_hook.py Outdated Show resolved Hide resolved

FifthPotato force-pushed the atomic_avu_handling_draft.m branch 2 times, most recently from 37d9dfb to 702a76b Compare February 1, 2024 17:16

FifthPotato force-pushed the atomic_avu_handling_draft.m branch 2 times, most recently from 321b06f to c95ec72 Compare February 1, 2024 17:25

korydraughn approved these changes Feb 1, 2024

View reviewed changes

[irods#38,irods#42] Handling for atomic metadata operations

92b99f5

metadata_guard now properly accounts for atomic metadata operations. Added unit tests, which use PRC.

FifthPotato force-pushed the atomic_avu_handling_draft.m branch from c95ec72 to 92b99f5 Compare February 1, 2024 17:38

alanking merged commit 6d9ce70 into irods:main Feb 1, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomic avu handling (main) #65

Atomic avu handling (main) #65

FifthPotato commented Jan 9, 2024

trel commented Jan 9, 2024

trel commented Jan 9, 2024

korydraughn commented Jan 9, 2024

trel commented Jan 9, 2024

alanking commented Jan 16, 2024

korydraughn left a comment

alanking left a comment

korydraughn left a comment

alanking left a comment

trel commented Jan 25, 2024

alanking left a comment

alanking left a comment

korydraughn left a comment

korydraughn left a comment

FifthPotato commented Jan 31, 2024

korydraughn commented Jan 31, 2024

korydraughn left a comment

FifthPotato commented Jan 31, 2024

alanking commented Jan 31, 2024

korydraughn commented Jan 31, 2024

alanking commented Jan 31, 2024

korydraughn commented Jan 31, 2024

korydraughn commented Feb 1, 2024

korydraughn commented Feb 1, 2024

FifthPotato commented Feb 1, 2024

korydraughn left a comment

FifthPotato commented Feb 1, 2024

Atomic avu handling (main) #65

Atomic avu handling (main) #65

Conversation

FifthPotato commented Jan 9, 2024

trel commented Jan 9, 2024

trel commented Jan 9, 2024

korydraughn commented Jan 9, 2024

trel commented Jan 9, 2024

alanking commented Jan 16, 2024

korydraughn left a comment

Choose a reason for hiding this comment

alanking left a comment

Choose a reason for hiding this comment

korydraughn left a comment

Choose a reason for hiding this comment

alanking left a comment

Choose a reason for hiding this comment

trel commented Jan 25, 2024

alanking left a comment

Choose a reason for hiding this comment

alanking left a comment

Choose a reason for hiding this comment

korydraughn left a comment

Choose a reason for hiding this comment

korydraughn left a comment

Choose a reason for hiding this comment

FifthPotato commented Jan 31, 2024

korydraughn commented Jan 31, 2024

korydraughn left a comment

Choose a reason for hiding this comment

FifthPotato commented Jan 31, 2024

alanking commented Jan 31, 2024

korydraughn commented Jan 31, 2024

alanking commented Jan 31, 2024

korydraughn commented Jan 31, 2024

korydraughn commented Feb 1, 2024

korydraughn commented Feb 1, 2024

FifthPotato commented Feb 1, 2024

korydraughn left a comment

Choose a reason for hiding this comment

FifthPotato commented Feb 1, 2024