irods / irods_rule_engine_plugin_audit_amqp

BSD 3-Clause "New" or "Revised" License
2 stars 14 forks source link

[42,73,102,104,110] Modernization and refactoring #105

Closed SwooshyCueb closed 1 year ago

SwooshyCueb commented 2 years ago

Addresses #42 Addresses #73 Addresses #102 Addresses #104 Addresses #110 In service of #95 In service of #103 Supersedes #96 Supersedes #97

This PR has a lot:

trel commented 2 years ago

if this confirms the new qpid-proton packages are good-to-go, i'll get them pushed to packages.irods.org

korydraughn commented 2 years ago

Let's avoid adding the clang-format/tidy files to other projects until we figure out the reusable github workflows. I should have something ready regarding that soon.

korydraughn commented 2 years ago

Let's replace the use of "Resolves" word in the first comment of this PR so that the issues aren't automatically closed.

SwooshyCueb commented 2 years ago

Note to self: add a tick to "maximum oversquash" on the whiteboard

SwooshyCueb commented 1 year ago

How do we feel about main.cpp for the plugin filename? I think I'm okay with it, but just wanted to bring it up for discussion as we try to think about conventions in our plugins.

I'm not married to it. I can change it to audit_amqp.cpp if that is preferred.

korydraughn commented 1 year ago

I think main.cpp is a very clear indicator of where someone should look when coming to an unfamiliar project. I prefer main.cpp, but am not against other ideas if they prove more helpful for us/others.

Logical quotas, metadata guard, and hard links REPs also use main.cpp too.

trel commented 1 year ago

Logical quotas, metadata guard, and hard links REPs also use main.cpp too.

seems main.cpp is both convention and expected

alanking commented 1 year ago

main.cpp seems fine to me, then!

SwooshyCueb commented 1 year ago

Addressed all review comments. 96bca7c01065085129357e13643b305024876881 might be of interest

korydraughn commented 1 year ago

96bca7c looks fine overall. The only thing that stood out was the use of the C-style cast. A static_cast would be safer and better for tools. There may be an integer suffix for that too.

Other than that, was there anything in particular you feel we need to look at regarding that commit?

korydraughn commented 1 year ago

Sadly, the integer suffix zu isn't available until C++23 (or with -std=c++2b).

SwooshyCueb commented 1 year ago

Switched it to a static_cast and squashed. If there are no objections, I'll go ahead and #

korydraughn commented 1 year ago

Do all of the tests pass (python, cpp unit tests)?

SwooshyCueb commented 1 year ago

cpp unit test suite passes. python test suites don't. looks like I've still got some work to do

SwooshyCueb commented 1 year ago

Removed stomp-based python tests (no use updating since everything they covered is already covered by the proton-based python tests) and updated the proton-based python tests. All tests pass except one pulled from the unixfilesystem tests that appears unrelated.

SwooshyCueb commented 1 year ago

If there are no objections, I will #

korydraughn commented 1 year ago

Is there one or more issues we can link the commits without an issue number to?

If not, let's create issues for them. Leave off the pounds so we can eyeball them one last time.

I'm assuming all of the tests pass now (I think you mentioned that this morning?).

trel commented 1 year ago

yes, definitely need some squashing or issuing...

SwooshyCueb commented 1 year ago

Missing issue numbers added. I don't want to squash any more than I already have, as I think it's potentially useful to be able to follow the commit history here.

I'm assuming all of the tests pass now (I think you mentioned that this morning?).

My earlier comment still applies:

All tests pass except one pulled from the unixfilesystem tests that appears unrelated.

SwooshyCueb commented 1 year ago

#'d

SwooshyCueb commented 1 year ago

Fixed the missing #, and added a change I forgot to include in the last commit (postinst script). Should be ready for merge now. Running tests to find the failing unixfilesystem test, will open an issue for it once I find it

korydraughn commented 1 year ago

Don't forget to update/close the issues. Ping @alanking if you need a checkbox checked.