Skip to content

[#525] Add support for touch API operation#531

Merged
alanking merged 1 commit into
irods:mainfrom
korydraughn:525
Jun 12, 2024
Merged

[#525] Add support for touch API operation#531
alanking merged 1 commit into
irods:mainfrom
korydraughn:525

Conversation

@korydraughn
Copy link
Copy Markdown
Contributor

@korydraughn korydraughn commented Apr 5, 2024

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
Copy Markdown
Member

trel commented Apr 6, 2024

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

@korydraughn
Copy link
Copy Markdown
Contributor Author

korydraughn commented Apr 6, 2024

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

d-w-moore
d-w-moore approved these changes Apr 6, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@d-w-moore d-w-moore left a comment •
edited
Loading

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

Comment thread irods/manager/collection_manager.py
Comment thread irods/manager/common.py Outdated
@d-w-moore
Copy link
Copy Markdown
Collaborator

d-w-moore commented Apr 6, 2024

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
Copy Markdown
Contributor Author

korydraughn commented Apr 7, 2024

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
Copy Markdown
Contributor Author

korydraughn commented Apr 7, 2024

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

@korydraughn
Copy link
Copy Markdown
Contributor Author

korydraughn commented Apr 7, 2024

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

trel
trel reviewed May 7, 2024
View reviewed changes
Comment thread irods/test/collection_test.py Outdated
alanking
alanking reviewed May 13, 2024
View reviewed changes
Copy link
Copy Markdown
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

Comment thread irods/manager/collection_manager.py Outdated
Comment thread irods/manager/collection_manager.py
Comment thread irods/manager/common.py Outdated
Comment thread irods/manager/common.py Outdated
Comment thread irods/manager/data_object_manager.py Outdated
Comment thread irods/manager/collection_manager.py
Comment thread irods/test/collection_test.py Outdated
Comment thread irods/test/collection_test.py Outdated
Comment thread irods/test/data_obj_test.py Outdated
Comment thread irods/test/data_obj_test.py
korydraughn
korydraughn commented May 13, 2024
View reviewed changes
Comment thread irods/test/collection_test.py
Comment thread irods/test/collection_test.py Outdated
Comment thread irods/test/collection_test.py Outdated
Comment thread irods/test/collection_test.py Outdated
@korydraughn
Copy link
Copy Markdown
Contributor Author

korydraughn commented May 16, 2024 •
edited
Loading

All review comments addressed.

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

alanking
alanking reviewed May 17, 2024
View reviewed changes
Copy link
Copy Markdown
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

Comment thread irods/manager/util/generic_api_endpoint.py Outdated
alanking
alanking reviewed Jun 10, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

korydraughn commented Jun 11, 2024

Squashed.

alanking
alanking approved these changes Jun 11, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

korydraughn commented Jun 11, 2024

Running full test suite to make sure nothings broken.

Will pound if everything comes out good.

@korydraughn
Copy link
Copy Markdown
Contributor Author

korydraughn commented Jun 11, 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.

@d-w-moore
Copy link
Copy Markdown
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.

@d-w-moore
Copy link
Copy Markdown
Collaborator

d-w-moore commented Jun 12, 2024 •
edited
Loading

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?

This was referenced Jun 12, 2024
Closed
Closed
d-w-moore
d-w-moore approved these changes Jun 12, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@d-w-moore d-w-moore left a comment •
edited
Loading

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
Copy Markdown
Contributor Author

korydraughn commented Jun 12, 2024

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

Reviewers

@trel trel trel left review comments

@alanking alanking alanking approved these changes

+1 more reviewer

@d-w-moore d-w-moore d-w-moore approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@korydraughn @trel @d-w-moore @alanking