search

uptane / aktualizr

C++ Uptane Client
Mozilla Public License 2.0
15 stars 15 forks source link

Shellcheck fixes #62

Closed pattivacek closed 2 years ago

pattivacek commented 2 years ago

Still the usual p11 errors, plus test_aktualizr_kill is being flaky again. I don't know what's up with that.

cajun-rat commented 2 years ago

Git bisect says that #4c1c3f04 "Don't EXIT_SUCCESS if Aktualizr::RunForever() throws an exception" is the culprit. Which apparently I wrote. Mea Culpa.

cajun-rat commented 2 years ago

This looks good overall. Given that these are probably not very well covered by unit tests and the changes were (I assume) manual, I'd like to review it line-by-line with a clear head and not at the end of a long week. The few I checked were all good though.

Architecturally speaking, it would be nice if we could run these tests locally in ctest -- things that only run in CI eventually force you to do a 'commit; push; wait for CI' cycle. I don't think that is a reason to push back against this PR going in, and it should be pretty straightforward for some who cares (i.e. me :) to implement aktualizr_shell_file_checks that creates relevant tests if shellcheck is installed.

cajun-rat commented 2 years ago

Fix to test_aktualizr_kill flakiness in #64

pattivacek commented 2 years ago

Given that these are probably not very well covered by unit tests

I think, with the exception of the start.sh script copied from OTA community edition (which is probably broken and/or obsolete anyway), most of these scripts are explicitly used only for unit tests.

the changes were (I assume) manual

About half and half, FWIW.

Architecturally speaking, it would be nice if we could run these tests locally in ctest -- things that only run in CI eventually force you to do a 'commit; push; wait for CI' cycle. I don't think that is a reason to push back against this PR going in, and it should be pretty straightforward for some who cares (i.e. me :) to implement aktualizr_shell_file_checks that creates relevant tests if shellcheck is installed.

That's a great point. It shouldn't be hard to do; we could just have a cmake command to run the same command that I described in the commit log (find docs/ scripts/ src/ tests/ -name "*.sh" | xargs shellcheck).

I was also considering a similar flake8 job.

pattivacek commented 2 years ago

All looks good!

Thanks!

Reminds me what a terrible language bash is: It seems be be designed with the goal of making the straightforward way of writing anything subtly incorrect, while still working for the majority of cases.

Yeah, no kidding. It's amazing how easy it is to write subtly broken code. Tons of special characters and hidden meanings, plus debugging it is super fun.