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

ECKIT-619 Enable PUT method on EasyCURL #81

Merged
merged 4 commits into from
Nov 18, 2023

Conversation

marcosbento
Copy link
Contributor

This pull request provides a readCallback to enable uploading the HTTP PUT request body, and updates the EasyCURL::PUT to effectively process the PUT method request.

@FussyDuck
Copy link

FussyDuck commented Sep 1, 2023

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (ffdee81) 62.85% compared to head (9fc8e83) 62.17%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #81      +/-   ##
===========================================
- Coverage    62.85%   62.17%   -0.68%     
===========================================
  Files          796      784      -12     
  Lines        45287    44467     -820     
===========================================
- Hits         28463    27647     -816     
+ Misses       16824    16820       -4     
Files Coverage Δ
src/eckit/io/EasyCURL.cc 0.00% <0.00%> (ø)

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tlmquintino tlmquintino 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 we add a test please? or is there no tests for the curl?
I think we need to add tests that then could use our nexus servers to test the code both for gets and puts.

@marcosbento
Copy link
Contributor Author

Looks good. Can we add a test please? or is there no tests for the curl? I think we need to add tests that then could use our nexus servers to test the code both for gets and puts.

Currently there are no tests for CURL, but I'll definitely have a look at your suggestion regarding using our Nexus servers as target end points.

A specific test file was created containing the existing EasyCURL test.
The existing EasyCURL test itself was updated to correct the URL used to access the test data at ECMWF's Nexus Repository.
@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Sep 19, 2023
@iainrussell iainrussell added the approved-for-ci Approved for CI run label Sep 19, 2023
Copy link
Contributor

@simondsmart simondsmart left a comment

Choose a reason for hiding this comment

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

This looks good overall.

Ideally I would like to see a GET/PUT test that fails/succeeds as appropriate.

curl.headers(headers);

auto response = curl.GET("https://get.ecmwf.int/service/rest/v1/assets?repository=test-data");
EXPECT(response.code() == 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have some GET tests that fail (i.e. not just code == 200)

curl.headers(headers);

auto response = curl.PUT("https://get.ecmwf.int/service/rest/v1/security/anonymous", "");
EXPECT(response.code() == 403);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance of a successful PUT request? Nothing here demonstrates that the data is being passed through properly.

Ideally a PUT request that can re-retrieve the data, and compare.

Copy link
Contributor Author

@marcosbento marcosbento Sep 20, 2023

Choose a reason for hiding this comment

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

I'm consulting with @iainrussell to see if there is any way to make a PUT request on our Nexus, without needing authentication (i.e. with public access).

If you have any idea, let us know.

The tests exercise EasyCURL calls for GET, POST, PUT and DELETE.

Existing and new tests are supported by a Python-based Mock REST API that stores key+value pairs.
A Python environment with Flask is necessary to run the Mock REST API.
@github-actions github-actions bot removed the approved-for-ci Approved for CI run label Oct 4, 2023
@iainrussell iainrussell added the approved-for-ci Approved for CI run label Oct 4, 2023
@marcosbento
Copy link
Contributor Author

Hi @simondsmart , @tlmquintino

I've added a more elaborate test (covering GET, POST, PUT and DELETE methods) , based on a mock REST API implemented in Python+Flask. Can you please have another look?

@marcosbento
Copy link
Contributor Author

Hi @simondsmart , @tlmquintino

Are you happy to merge this PR now?

@danovaro danovaro merged commit 0f21b65 into ecmwf:develop Nov 18, 2023
128 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants