[#128] Update for iRODS 4.3.1 (main) #131

korydraughn · 2024-02-15T21:33:08Z

Aside from a few recent changes, I've seen this code pass the test suite against Elasticsearch 7.4.2 and 8.12.1.

Below are the test results for Elasticsearch 8.12.1. Notice the atomic metadata ops test was skipped. I think this is mostly fine given the fact that the atomic APIs are relatively stable. I'll see if I can track down how to properly handle that test case.

workstation:~/scripts$ time python3 run_tests.py --run_s test_plugin_indexing
irods.test.test_plugin_indexing.TestIndexingPlugin.test_applying_indicators_ex_post_facto ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_fulltext_and_metadata_indicators_on_same_colln__19 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_graceful_handling_of_premature_deletion_62 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_indexing_01_basic ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_indexing_of_odd_chars_and_json_in_metadata__41__43 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_indexing_with_atomic_metadata_ops_66 ... skipped 'Only manual testing is enabled for tests that depend on a Python3 virtual env'
irods.test.test_plugin_indexing.TestIndexingPlugin.test_iput_with_metadata__92 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_load_parameters_as_int_or_string__91 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_purge_when_AVU_indicator_removed ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_response_to_elasticsearch_404_fulltext__issue_34 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_response_to_elasticsearch_404_metadata__issue_34 ... ok
irods.test.test_plugin_indexing.TestIndexingPlugin.test_throttle_limit_45 ... ok

----------------------------------------------------------------------
Ran 12 tests in 1088.884s

OK (skipped=1)
<__main__.RegisteredTestResult run=12 errors=0 failures=0>

real    18m11.630s
user    0m39.715s
sys     0m22.166s

Now we need to think through how to incorporate testing of Elasticsearch 8. To test it, I launched an Elasticsearch container and ran the tests at the bench. We'll need to decide whether it's worth modifying the test hook or moving towards a docker compose like setup.

Forgot to mention - this PR also formats all of the C++ code. It's better to view the commits individually so you see what the true changes for 4.3.1 are.

configuration.cpp

configuration.hpp

cpp_json_kw.hpp

libirods_rule_engine_plugin-elasticsearch.cpp

utilities.hpp

plugin_specific_configuration.cpp

libirods_rule_engine_plugin-elasticsearch.cpp

alanking

Any guidance on how to review this would be helpful, given its size and the fact that not all of it is new. I've mainly been responding to your comments that solicit responses :) Thanks

configuration.hpp

trel · 2024-02-16T18:17:38Z

commit e42cc22 has typo in commit message (elasticlist)

libirods_rule_engine_plugin-elasticsearch.cpp

libirods_rule_engine_plugin-document_type.cpp

libirods_rule_engine_plugin-elasticsearch.cpp

trel · 2024-02-16T18:42:18Z

Any guidance on how to review this would be helpful

The bulk of the line changes are formatting. So clicking through the commits one by one made it easier to see what was going on / ignore the clang formatting.

korydraughn · 2024-02-16T19:23:02Z

Right. Definitely better to view each commit separately.

Once I resolve the current comments, I'll squash and it should be easier to read.

I'll put this in draft for now.

alanking

Hopefully some of these aren't out of date...

document_type.cmake

packaging/test_plugin_indexing.py

libirods_rule_engine_plugin-elasticsearch.cpp

korydraughn · 2024-02-20T14:17:20Z

NTS: Test TLS support.

korydraughn · 2024-02-20T14:43:09Z

Noting here that TLS with ES requires authentication. I don't think this plugin currently supports configuration options for that.

Also, the ESv8 docker container generates the necessary certs for you to test it. You can read about it here.

https://www.elastic.co/guide/en/elasticsearch/reference/current/docker.html#docker-cli-run-dev-mode

I think TLS support will need to happen in a separate PR or later release.

trel · 2024-02-20T14:49:20Z

I think TLS support will need to happen in a separate PR or later release.

That's okay - we can get it working/compiling/happy with the latest other things, and get TLS added quick as soon as someone needs it.

korydraughn · 2024-02-20T15:57:23Z

Apparently we do have an issue for TLS. See #112

korydraughn · 2024-02-20T20:19:09Z

After some discussion, we've decided to remove the document_type REP. The reason behind this change is because elasticsearch no longer supports mapping types.

For more information, see https://www.elastic.co/guide/en/elasticsearch/reference/7.17/removal-of-types.html.

korydraughn · 2024-02-21T05:03:10Z

Cleaned up commits.

Running tests.

korydraughn · 2024-02-21T14:08:15Z

All tests pass (minus the atomic avu one).

Tested using the testing environment (ES v7.4).
Also tested outside of the testing environment using containers running ES v7.17 and ES v8.12.

korydraughn · 2024-02-21T15:29:36Z

Ran the tests again to make sure things work after that last commit.

I need to revisit the postinst script commit though.

alanking · 2024-02-21T18:54:33Z

Everything seems good to me so far. Will await word on the postinst script.

korydraughn · 2024-02-21T20:13:40Z

Perhaps removing those references to the postinst script is better since the cmake file defines the following.

irods_capability_indexing/CMakeLists.txt

Lines 104 to 112 in 82eee54

    
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${CMAKE_INSTALL_LIBDIR}/irods") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${IRODS_PLUGINS_DIRECTORY}") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${IRODS_PLUGINS_DIRECTORY}/rule_engines") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${CMAKE_INSTALL_LOCALSTATEDIR}") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${CMAKE_INSTALL_LOCALSTATEDIR}/lib") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${IRODS_HOME_DIRECTORY}") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${IRODS_HOME_DIRECTORY}/scripts") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${IRODS_HOME_DIRECTORY}/scripts/irods") 
        
           list(APPEND CPACK_RPM_EXCLUDE_FROM_AUTO_FILELIST_ADDITION "${CPACK_PACKAGING_INSTALL_PREFIX}${IRODS_HOME_DIRECTORY}/scripts/irods/test")

Will run the tests for CentOS 7 and the Almalinux / Rocky Linux to verify.

korydraughn · 2024-02-21T23:24:19Z

I've confirmed the changes pass the tests for CentOS 7.

I cannot run the tests for Almalinux 8 and Rocky Linux 9 due to docker and python pip issues. Regardless, I was able to confirm the packages produced by this PR install without issues for these platforms.

All I have to do now is assign issues and we're good to go.

korydraughn · 2024-02-21T23:39:36Z

Issue numbers assigned.

Just need that good ole approval.

README.md

alanking

Seems good to me. Let's do it

…pts.

…fails.

…pts.

korydraughn · 2024-02-22T15:09:30Z

Added #'s

korydraughn commented Feb 15, 2024

View reviewed changes