irods / irods_capability_automated_ingest

Other
12 stars 16 forks source link

Fix exclude_file_name being ignored #182

Closed alanking closed 1 year ago

alanking commented 1 year ago

Due to the recent refactor and lack of test coverage in this area, --exclude_file_name has been broken since 0.4.0. This is because the implementation moved control blocks but was not updated properly to adapt to its new context. This fix is not a complete solution, but allows the option to be used as expected once again.

korydraughn commented 1 year ago

Anything keeping this from moving forward?

alanking commented 1 year ago

Yes. I feel that this implementation is incomplete. It treats the symptom, but the problem is more with the interface, so I will need to take another pass at this.

rmoreas commented 1 year ago

I need a fix for this urgently. IMHO this would be a safe quick fix for now.

rmoreas commented 1 year ago

@alanking: would be nice if the new patch release also includes the fix #202

/cc @trel

trel commented 1 year ago

we're going to look today and work to get a new release out.

candidates at the moment: #182, #202, and #194

alanking commented 1 year ago

I added a test and confirmed that the issue is fixed. We will need more expansive tests around this and other sync options in the future.

trel commented 1 year ago

What changed in the last day or so?

Last I knew we weren't sure if this PR was a real red->green fix.

rmoreas commented 1 year ago

What changed in the last day or so?

Last I knew we weren't sure if this PR was a real red->green fix.

It's because of me 🙂 I did have a talk about this with Alan last week because we need this bug fix for a project urgently. I understand that the current implementation of this feature is maybe not optimal (it would be better with PEP's as suggested in #184), but we can't wait for a redesign. This PR is an easy fix for this bug, without any risks.

alanking commented 1 year ago

Right, the problem was that the data objects were being registered with the old code, so it was difficult to observe the failure. I added an assertion that the "failed" queue had no items in it. This assertion fails appropriately without the fix, and succeeds with it.

trel commented 1 year ago

perfect. thanks.