Skip to content

Conversation

@korydraughn
Copy link
Contributor

The implementation appears to work.

Just need to implement tests.

@korydraughn korydraughn requested a review from d-w-moore April 5, 2024 23:10
@trel
Copy link
Member

trel commented Apr 6, 2024

Why all caps error code? Is there precedent for that in PRC?

@korydraughn
Copy link
Contributor Author

Just not familiar with all the conventions yet. Also, I think I saw exceptions like that used in other places in the PRC.

Copy link
Collaborator

@d-w-moore d-w-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is well done. I like the use of an irods.manager.common for commonly /multiply used code. Wonder now if we should scan for instances where irods.common would be similarly useful.

Approve the changes so far. Guess we'll have one more round once tests are implemented

@d-w-moore
Copy link
Collaborator

Why all caps error code? Is there precedent for that in PRC?

I don't know the valid uses of INVALID_OBJECT_TYPE , either. Not currently used otherwise, so may be a relic of something in PRC's past that got deleted. It appears also that it subclasses RuleEngineException, which may be misleading. I would suggest another error (possibly more targeted? Invalid_Path_For_Object_Type?)

@korydraughn
Copy link
Contributor Author

I added a new exception type, InvalidInputArgument, to replace the use of INVALID_OBJECT_TYPE.

All new tests pass. The only way I could run my tests was by skipping the iRODS version check code in helpers.make_session(). Is there another way to handle that?

@korydraughn korydraughn marked this pull request as ready for review April 7, 2024 15:28
@korydraughn
Copy link
Contributor Author

Forgot to mention this PR also adds modify_time and create_time to the replica object. See aad7740.

@korydraughn
Copy link
Contributor Author

Looks like I need to address a few more codacy issues.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meat of the change is good. Just a few suggestions/questions

@korydraughn
Copy link
Contributor Author

korydraughn commented May 16, 2024

All review comments addressed.

All that's left is to run the full test suite.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had one more suggestion. Looks good otherwise

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Please squash to taste and omit #s.

@korydraughn
Copy link
Contributor Author

Squashed.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# it

@korydraughn
Copy link
Contributor Author

Running full test suite to make sure nothings broken.

Will pound if everything comes out good.

@korydraughn
Copy link
Contributor Author

I saw 2 failures and 46 errors. Not sure what to expect.

@d-w-moore Any thoughts? Please try running this PR through the test suite and see what you get.

@d-w-moore
Copy link
Collaborator

I saw 2 failures and 46 errors. Not sure what to expect.

@d-w-moore Any thoughts? Please try running this PR through the test suite and see what you get.

Sure, will run now.

@d-w-moore
Copy link
Collaborator

d-w-moore commented Jun 12, 2024

I saw 2 failures and 46 errors. Not sure what to expect.
@d-w-moore Any thoughts? Please try running this PR through the test suite and see what you get.

Sure, will run now.
Got 13 errors from the entire suite. (270 tests run), running against iRODS 4.3.2.
All 13 accounted for by:

  • errors coming from login_auth_test.py. It requires specific setups, so shouldn't be run as a matter of course in the PRC suite. Unfortunate bad form on that one, mea culpa. That will be fixed this next release (see issue Investigate automating BATS tests #502. we'll be renaming it to run in a BATS test and that should defeat the pattern matching that causes it to be picked up in the routine run of the full suite).
  • irods.message.Bad_AVU_Value should be descended from ValueError for irods.test.meta_test.TestMeta.test_add_obj_meta_empty to pass.

@korydraughn - For other errors you may have seen, we should discuss live. Do you have a log dump?

Copy link
Collaborator

@d-w-moore d-w-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, can go ahead and #. I've tested using https://github.com/d-w-moore/python-irodsclient/tree/ghrunner-nohealthcheck (currently one of my development branches at ) as we discussed.

@korydraughn
Copy link
Contributor Author

Excellent.

#'d

@alanking alanking merged commit c3963c2 into irods:main Jun 12, 2024
@korydraughn korydraughn deleted the 525 branch June 12, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants