Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to iRODS HTTP API #46

Merged
merged 9 commits into from
Mar 15, 2024
Merged

Switch to iRODS HTTP API #46

merged 9 commits into from
Mar 15, 2024

Conversation

MartinSchobben
Copy link
Collaborator

@MartinSchobben MartinSchobben commented Jan 26, 2024

This updates rirods to version 0.2.0 which is based on iRODS HTTP API 0.2.0.

Things to consider:

  • Wait for new HTTP API release (0.2.0), this will e.g. remove count parameter from write endpoint
  • Documentation has only been roughly adapted for now
  • Only the old rirods 0.1.2 functionality has now been implemented with some minor addition, such as, the server-info endpoint

@MartinSchobben MartinSchobben linked an issue Jan 26, 2024 that may be closed by this pull request
@MartinSchobben MartinSchobben self-assigned this Jan 26, 2024
R/collections.R Outdated Show resolved Hide resolved
@MartinSchobben
Copy link
Collaborator Author

I think jsonlite should be moved to Imports in DESCRIPTION. It somehow evaded the R CMD checks.

R/data-objects.R Outdated Show resolved Hide resolved
@MartinSchobben
Copy link
Collaborator Author

@montesmariana. Thanks for already looking into this! There are a lot of consideration again with the new API. Perhaps we can once go through this over a video call.

@korydraughn
Copy link

Given the number of files being modified in this PR and me not being familiar with R, are there files we can/should ignore?

Are there files you want us to focus on?

@MartinSchobben
Copy link
Collaborator Author

MartinSchobben commented Jan 26, 2024

Are there files you want us to focus on?

The division of commits is as follows.

  • The first commit involves the actual changes in R (only 23 files).
  • The second commit involves documentation of the previous R files.
  • The third commit are the unit tests.
  • The fourth commit are the HTTP snapshots for the former unit tests. They capture the HTTP response, so that one can test even without internet or iRODS server. (100s of files)
  • Lastly, automatically generated README

I mainly wanted to let you (iRODS) know that this is underway (draft PR), but it seems to work fine. I will in the next weeks discuss with Mariana the finer details of the R package update.

@korydraughn
Copy link

Got it. And thanks for the explanation. I somehow didn't think to look at the commits.

Good to hear things appear to work.

We just released v0.2.0 of the HTTP API. It's available via docker hub. See https://hub.docker.com/r/irods/irods_http_api/tags.

@MartinSchobben
Copy link
Collaborator Author

This is then also directly available through the irods_demo. Right?

@MartinSchobben
Copy link
Collaborator Author

I answered that my self. But there seems to be a problem:

From running docker compose up:

irods-client-http-api-1   | Traceback (most recent call last):
irods-client-http-api-1   |   File "<string>", line 1, in <module>
irods-client-http-api-1   |   File "/usr/local/lib/python3.10/dist-packages/jsonschema/validators.py", line 1312, in validate
irods-client-http-api-1   |     raise error
irods-client-http-api-1   | jsonschema.exceptions.ValidationError: 'max_size_of_request_body_in_bytes' is a required property
irods-client-http-api-1   | 
irods-client-http-api-1   | Failed validating 'required' in schema['properties']['http_server']['properties']['requests']:
irods-client-http-api-1   |     {'properties': {'max_size_of_request_body_in_bytes': {'minimum': 0,
irods-client-http-api-1   |                                                           'type': 'integer'},
irods-client-http-api-1   |                     'threads': {'minimum': 1, 'type': 'integer'},
irods-client-http-api-1   |                     'timeout_in_seconds': {'minimum': 1,
irods-client-http-api-1   |                                            'type': 'integer'}},
irods-client-http-api-1   |      'required': ['threads',
irods-client-http-api-1   |                   'max_size_of_request_body_in_bytes',
irods-client-http-api-1   |                   'timeout_in_seconds'],
irods-client-http-api-1   |      'type': 'object'}
irods-client-http-api-1   | 
irods-client-http-api-1   | On instance['http_server']['requests']:
irods-client-http-api-1   |     {'max_rbuffer_size_in_bytes': 8388608,
irods-client-http-api-1   |      'threads': 3,
irods-client-http-api-1   |      'timeout_in_seconds': 30}
irods-client-http-api-1   | Configuration failed validation.
irods-client-http-api-1   | Validating configuration file ...
irods-client-http-api-1   | No JSON schema file provided. Using default.
irods-client-http-api-1 exited with code 1

@MartinSchobben
Copy link
Collaborator Author

I guess this needs to be change now: https://github.com/irods/irods_demo/blob/main/irods_client_http_api/config.json

@korydraughn
Copy link

That's correct. The demo environment needs an update.

we'll get a fix in today.

@korydraughn
Copy link

Here's the PR that bumps the HTTP API to 0.2.0 in case you want to give it a try.

irods/irods_demo#59

Let us know if it works for you.

Copy link
Member

@trel trel left a comment

Choose a reason for hiding this comment

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

still a lot of duplication, but we can address that over time. the URL being everywhere feels very brittle.

DESCRIPTION Outdated Show resolved Hide resolved
R/admin.R Outdated Show resolved Hide resolved
R/admin.R Outdated Show resolved Hide resolved
R/admin.R Outdated Show resolved Hide resolved
R/admin.R Show resolved Hide resolved
R/metadata.R Outdated Show resolved Hide resolved
R/metadata.R Outdated Show resolved Hide resolved
R/metadata.R Outdated Show resolved Hide resolved
dev/Makefile Outdated Show resolved Hide resolved
inst/shell_scripts/iauth.sh Outdated Show resolved Hide resolved
@MartinSchobben
Copy link
Collaborator Author

still a lot of duplication, but we can address that over time. the URL being everywhere feels very brittle.

Not sure what is meant. I presume this can only be documentation and testing, using the localhost and the associated URL. Pretty sure there is no hardcoded URL.

@trel
Copy link
Member

trel commented Jan 29, 2024

still a lot of duplication, but we can address that over time. the URL being everywhere feels very brittle.

Not sure what is meant. I presume this can only be documentation and testing, using the localhost and the associated URL. Pretty sure there is no hardcoded URL.

I mean that I see the URL (and the version number) hard-coded in many places. When we bump to 0.3.0 or 1.0.0 we'll need to touch everything again. Was thinking perhaps there could be a single source of truth, and a variable that gets used everywhere. Not critical, an optimization, perhaps.

@MartinSchobben
Copy link
Collaborator Author

still a lot of duplication, but we can address that over time. the URL being everywhere feels very brittle.

Not sure what is meant. I presume this can only be documentation and testing, using the localhost and the associated URL. Pretty sure there is no hardcoded URL.

I mean that I see the URL (and the version number) hard-coded in many places. When we bump to 0.3.0 or 1.0.0 we'll need to touch everything again. Was thinking perhaps there could be a single source of truth, and a variable that gets used everywhere. Not critical, an optimization, perhaps.

I see. I guess you mainly see the duplication in the roxygen examples with 'create_irods'. I presume there must indeed be a smarter way to do that. I'll investigate.

@korydraughn
Copy link

@MartinSchobben We've merged irods/irods_demo#59. The HTTP API is pinned to 0.2.0 in irods/irods_demo until the next release.

@MartinSchobben
Copy link
Collaborator Author

MartinSchobben commented Feb 2, 2024

Well, it all seems to work with HTTP API 0.2.0. Also included now a dynamic way of generating the documentation in regards to the HTTP API version changes. Although the website does not look that nice for the Reference to functions: https://martinschobben.github.io/irods_client_library_rirods/index.html. Moreover I can't run one vignette against the irods_demo (local vs iRODS) due a problem with a dependency when capturing HTTP responses (so this does not relate to functionality of rirods).

@trel
Copy link
Member

trel commented Feb 2, 2024

I see three references to 'REST' in the rendered page at https://martinschobben.github.io/irods_client_library_rirods/index.html

@MartinSchobben MartinSchobben force-pushed the dev branch 4 times, most recently from 0f5e5c6 to 1cde369 Compare March 11, 2024 06:35
@MartinSchobben
Copy link
Collaborator Author

I think that the PR is nearing completion. See also the blog post draft: https://martinschobben.github.io/iRODS4R/posts/rirods-0-2-0/ for when it is accepted at CRAN. Please propose edits at the source: https://github.com/MartinSchobben/iRODS4R/tree/dev-blog.

@MartinSchobben MartinSchobben force-pushed the dev branch 2 times, most recently from ce7b842 to cd2ed0f Compare March 11, 2024 08:45
@trel
Copy link
Member

trel commented Mar 11, 2024

I think that the PR is nearing completion. See also the blog post draft: https://martinschobben.github.io/iRODS4R/posts/rirods-0-2-0/ for when it is accepted at CRAN. Please propose edits at the source: https://github.com/MartinSchobben/iRODS4R/tree/dev-blog.

please start a PR for the post, and then edits/comments can be added.

@MartinSchobben
Copy link
Collaborator Author

@MartinSchobben
Copy link
Collaborator Author

I asked @jspijker if they want to have a look at this PR at the RIVM, so maybe more changes will still follow.

@MartinSchobben
Copy link
Collaborator Author

I asked @jspijker if they want to have a look at this PR at the RIVM, so maybe more changes will still follow.

Okay, the tests by RIVM will have to wait a bit. So, I guess we can proceed. Or will more testing proceed at KU Leuven @montesmariana?

@trel
Copy link
Member

trel commented Mar 15, 2024

I see 9 commits.

Please confirm this is ready for merging as is.

@MartinSchobben
Copy link
Collaborator Author

This is ready to merge. Thanks

@trel trel merged commit 1301faf into irods:main Mar 15, 2024
@trel
Copy link
Member

trel commented Mar 15, 2024

We now have a candidate SHA for 0.2.0... 1301faf

Please confirm that SHA is correct for CRAN and GitHub release.

@MartinSchobben
Copy link
Collaborator Author

I need to make another tiny PR. CRAN checks complained about a possible unsafe URL.

@trel
Copy link
Member

trel commented Mar 15, 2024

got it.

@MartinSchobben
Copy link
Collaborator Author

@trel
Copy link
Member

trel commented Mar 15, 2024

Oh, excellent. Which SHA shall we tag as 0.2.0?

@trel
Copy link
Member

trel commented Mar 16, 2024

@MartinSchobben I assume defa2ca

please confirm.

@MartinSchobben
Copy link
Collaborator Author

Yes, that's correct

@trel
Copy link
Member

trel commented Mar 16, 2024

Got it - will tag and release here at GitHub.

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.

update to use the new iRODS HTTP API
4 participants