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

Fix partial success HTTP status codes #4266

Conversation

chetan-NEC
Copy link
Contributor

Fix #3054

  • Change response code for partially successful updated entities

@fgalan
Copy link
Member

fgalan commented Jan 11, 2023

Thanks you for your contribution!

Could you upgrade chetan-NEC:partial-success-http-status-codes branch which master branch, please? The CI docker image has been updated to use mongoc 1.23.1 and you need to bring to your PR the changes in CMakeList.txt and src/lib/rest/rest.cpp already in PR #4259 currently merged in master.

@chetan-NEC
Copy link
Contributor Author

Hi @fgalan, the branch has been upgraded.

@chetan-NEC
Copy link
Contributor Author

Gentle reminder!

@chetan-NEC
Copy link
Contributor Author

Hi @fgalan, Gentle reminder!

@chetan-NEC
Copy link
Contributor Author

Hi @fgalan, I have achieved the desired functionality, but NGSIv1-related test cases have failed. 
So, do you have any suggestions? 
Thank you in advance for your time and assistance. I look forward to reading your response.

@fgalan
Copy link
Member

fgalan commented Feb 10, 2023

I had some time to have a look to the PR. First of all, thank for your willingness to contribute.

However, I'm still concerned about the backward compatibility issues that I commented in the past.

So maybe it's not a good idea to do the kind of changes this PR is proposing. Maybe it was a good idea when issue #3054 was created times ago, but NGSIv2 API usage has consolidated and I think there is little gain in implementing that issues (versus a high impact in backward compability).

Thus, I'd propose the following:

  1. Close issue Partial success HTTP status codes #3054 explaining the above rationale
  2. Closing PR Fix partial success HTTP status codes #4266 (along with previous PR Fix Partial success HTTP status codes #3054 #4003)

@chetan-NEC what do you think, please?

@chetan-NEC
Copy link
Contributor Author

@fgalan, I spend lot of time to achieve the required functionality, but I really agree with your recommendation to close this PR.

@fgalan
Copy link
Member

fgalan commented Feb 13, 2023

@fgalan, I spend lot of time to achieve the required functionality, but I really agree with your recommendation to close this PR.

Your time has not been wasted :) It has been very useful to reach the conclusion about what to do with the issue, which is also very important.

Note also that even in the case we want to change the behaviour, the work would not be finished. New tests (the ones described here) would need to be implemented, which would involve more effort (with uncertain result).

Having said that, if you have a proposal on how to fix the issue without involving backward compatibility problems, please go ahead with the suggestion. My proposal of closing the issue along with related PRs is due to I don't see any good solution... but if somebody does please tell me about :)

Otherwise, please @chetan-NEC close PR #4266 and I'll go ahead with the remaning closing actions.

Thanks for your feedback!

@chetan-NEC
Copy link
Contributor Author

Thanks for your feedback!
I have closed the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial success HTTP status codes
2 participants