Add http.put() - Githubissues

marcelstoer commented 3 years ago

Fixes #3456.

[x] This PR is for the dev-esp32 branch rather than for the release branch.
[x] This PR is compliant with the other contributing guidelines as well (if not, please describe why).
[x] I have thoroughly tested my contribution.
[x] The code changes are reflected in the documentation at docs/*.

@jmattsson @tomsci I've had this laying around for quite some time and was wondering if you'd support adding it.

tomsci commented 3 years ago

Seems like a beneficial change. My only concern would be about removing the setpostdata() API. Should we also expose http_lapi_setbody as setpostdata in http_context to maintain compatibility?

marcelstoer commented 3 years ago

@tomsci I briefly considered this before putting up the PR but decided against it for two reasons

setpostdata() also set the method to POST; setbody() doesn't. Hence, we'd either have a breaking change anyway or we'd somehow have to iron this out.
dev-esp32 has always been a development branch without any guarantees as for API stability. That doesn't mean we should carelessly break APIs but IMO we don't have to be as strict as on the release branch.

marcelstoer commented 3 years ago

should we consider merging this only onto the idf4 branch, since we already have breaking changes there?

Sounds good to me.

marcelstoer commented 2 years ago

merging this only onto the idf4 branch

Ok, I changed the base branch. Will resolve the conflicts now.

jmattsson commented 2 years ago

Merged, thanks. Am I good to remove the feat/http-put branch?

jmattsson commented 2 years ago

Dear heavens now everything broke when I merged that?! But it showed all green for the merge?! Aaaah! I can't reproduce the build failure locally. @marcelstoer help, please? 😢

Edit: doesn't look like it's the merge that made things fail; re-running other previously-all-ok builds now also fails. Something must've changed with the test runner instances...

marcelstoer commented 2 years ago

Well, you couldn't have merged if the PR build hadn't succeeded. Here's that build: https://github.com/nodemcu/nodemcu-firmware/runs/3668105377 - I won't touch it as it's green now 😜

I'll take a look at the others.

marcelstoer commented 2 years ago

https://github.com/nodemcu/nodemcu-firmware/runs/3798787408#step:10:1249

gen_crt_bundle.py: Invalid certificate in /home/runner/work/nodemcu-firmware/nodemcu-firmware/sdk/esp32-esp-idf/components/mbedtls/esp_crt_bundle/cacrt_all.pem Invalid certificate

May have to bump the IDF again?

jmattsson commented 2 years ago

I considered it, but since the build isn't failing for me locally I'm wondering if the test runner host(s) aren't getting their time set properly for some reason all of a sudden?

marcelstoer commented 2 years ago

the build isn't failing for me locally

Right, you said so. Sorry I forgot.

aren't getting their time set properly

That would certainly explain it. https://www.githubstatus.com/ isn't aware (yet) of any issues with the runners.

We could temporarily add a few debugging commands to the build script. I assume you inspected cacrt_all.pem locally and found it to be valid, right?

jmattsson commented 2 years ago

I assume you inspected cacrt_all.pem locally and found it to be valid, right?

Only in so far that a make clean; make did not have it complain about it being invalid.

I also re-ran the github actions on one of my own branches, which had previously also been a fully green build, and it went all red on the re-run.

Please be my guest and see if you can find a workaround for this issue; I'm a bit occupied with other $work bits at the moment. I just thought I'd sneak in the re-review/merge of this PR before I had to get stuck into $work...

marcelstoer commented 2 years ago

This issue is now all over the place: https://github.com/espressif/esp-idf/issues/5322#issuecomment-931150764, others, and https://github.com/espressif/esp-idf/issues/7631#issuecomment-934212224.

marcelstoer commented 2 years ago

I addressed this with 32f66e3 but I wasn't (and still am not) sure whether pinning the cryptography version was more appropriate for our project than putting CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=n into sdkconfig.defaults.

jmattsson commented 2 years ago

Thanks @marcelstoer, I really appreciate you taking the time to look into this one! Going by Ivan's comment, it looks like in the slightly longer term we can fix this with an update of the IDF. In the meantime I have no issue with the version lock, as for now it's not making things any worse than they already were.

nodemcu / nodemcu-firmware

Add http.put() #3462