api: reorganize keystore API

[kyrofa]

We don't currently have a nicely controlled/organized API: everything is implemented in the api's __init__.py, making is way longer than it needs to be, and most is public without needing to be, which makes it hard to change once we release. This PR starts the process of resolving that by extracting the keystore API into its own succinct, non-public module. In the end we'll hopefully have a tight, small public API to maintain.

Note that this introduces no behavioral changes, it's just moving code around.

[kyrofa]

Hmm, having some issues uploading the coverage it seems...

Error: connect ETIMEDOUT 35.199.43.247:443
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1129:14) {
  errno: 'ETIMEDOUT',
  code: 'ETIMEDOUT',
  syscall: 'connect',
  address: '35.199.43.247',
  port: 443
}

The tests pass! Trust me 😉 . I'll try re-running in a bit. Update: Aw heck, now the other one is failing to check the code out. Maybe github is having issues at the moment.

[Arnatious]

Since we're refactoring everything anyway, would it make sense to sub in pathlib for readability?

[kyrofa]

That's the thing though-- we're not actually refactoring anything here. That would require a more in-depth review and this simple re-organization can already be a pretty significant diff to look through. As it stands I can promise that no meaningful changes were made, which makes review a bit faster assuming the reviewer trusts me. Once this re-organization is finished, we can do all sorts of things (including changing to pathlib) without breaking API, so that isn't time-sensitive.

[ruffsl]

Nice, I was disliking that long file. Just built and ran this locally on osrf/ros2:nightly and all seems well.

[mikaelarguedas]

I'll try re-running in a bit. Update: Aw heck, now the other one is failing to check the code out. Maybe github is having issues at the moment.

These seem to be more of a github actions issue than a github outage issue...
A hidden issue that is happening (coverage still not being uploaded..) is that github actions doesnt populate the pull request name properly on reruns... actions/checkout#58 (comment)

hopefully most of these issues will be fixes by #189 🤞

Otherwise just amending you commig would make this a new workflow run and work around the issues related to "rerunning a failed workflow"

[mikaelarguedas]

Thanks for starting the refactor !

History should be preserved when refactoring. Right now all history is lost making it hard to use blame to get more info about a particular piece of code

General question: the new modules are prefixed with _, does that make them private?
In these (private?) modules some functions are private and some public, what does that make them ? all private?

[Arnatious]

@kyrofa per @mikaelarguedas request https://stackoverflow.com/questions/38729453/how-to-preserve-git-history-when-refactoring-into-multiple-files

[codecov]

Codecov Report

Merging #188 into master will increase coverage by 1.38%.
The diff coverage is 84.56%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   53.38%   54.76%   +1.38%     
==========================================
  Files          12       14       +2     
  Lines         562      577      +15     
  Branches       52       52              
==========================================
+ Hits          300      316      +16     
+ Misses        248      247       -1     
  Partials       14       14              

Flag	Coverage Δ
#unittests	54.76% <84.56%> (+1.38%)	⬆️

Impacted Files	Coverage Δ
sros2/sros2/api/__init__.py	64.88% <65.21%> (-8.57%)	⬇️
sros2/sros2/api/_keystore.py	84.72% <84.72%> (ø)
sros2/sros2/api/_utilities.py	90.76% <90.76%> (ø)
sros2/sros2/verb/create_keystore.py	78.57% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6480fe2...f1c7711. Read the comment docs.

[kyrofa]

General question: the new modules are prefixed with _, does that make them private?

Yes, although given that nothing is truly private in python, these are simply "non-public". It's simply a way of telling users what we're promising to maintain through a release and what might change.

In these (private?) modules some functions are private and some public, what does that make them ? all private?

It's the same public/non-public idea applied to the module. Underscore-prefixed things are intended for use within the module. Ideally such things would be able to change without needing to change any callers (other than the occasional test). The non-underscore-prefixed functions are intended for use outside of the module (e.g. by verbs). We could be using __all__ as well, although I'm not sure it gains us much.

Anyway, in this PR the modified verbs are now using non-public API. That's okay because they're in the same package, and even the non-public modules have their own public/non-public API nicely defined. This comes in handy when it comes to external packages using the SROS2 API: this PR makes the keystore stuff all non-public, because the api package no longer contains any public keystore-related stuff. That's not necessarily the end goal, but this series of PRs will refactor it all to be non-public and we can then chat about what we actually want to support as public API for the LTS. Sound okay?

[mikaelarguedas]

this PR makes the keystore stuff all non-public, because the api package no longer contains any public keystore-related stuff. That's not necessarily the end goal, but this series of PRs will refactor it all to be non-public and we can then chat about what we actually want to support as public API for the LTS (probably by importing it back into init.py). Sound okay?

Sounds good to me 👍

---

Looks like the splitting with history didn't succeed, if I try to git blame the file I dont see the commits before the split: https://github.com/ros2/sros2/blame/300435a668e02bfa0118a2302621d169799afe06/sros2/sros2/api/_keystore.py

I was more expecting something like https://github.com/mikaelarguedas/sros2/blame/split-with-history/sros2/sros2/api/_keystore.py.

I can push a version with fixed history on this branch if you want

[kyrofa]

@kyrofa per @mikaelarguedas request https://stackoverflow.com/questions/38729453/how-to-preserve-git-history-when-refactoring-into-multiple-files

Thank you-- sadly it wasn't that easy!

Looks like the splitting with history didn't succeed

Indeed, that was a lot harder than it should have been, but I think I finally got it. Blames look way better now, no special flags needed.

[mikaelarguedas]

looks much nicer 👍, thanks for iterating

[kyrofa]

Hmm, something broke here again @mikaelarguedas, any ideas?

CMake Error at /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
    Could NOT find PythonLibs (missing: PYTHON_LIBRARIES PYTHON_INCLUDE_DIRS)
    (Required is at least version "3.5")

[mikaelarguedas]

Hmm, something broke here again @mikaelarguedas, any ideas?

Yes this is the same breakage as the daily sros2 builds that we mentioned on matrix. Issue is outside this repo, a workaround has been pushed and this is now fixed

TL;DR:
This is a combination of a change in the docker images, a bug of rosdep and some unexpected side effect of the new release of rosdep that all happened in the same 24 hours.
docker_images fix for new release of rosdep osrf/docker_templates#85
rosdep bug tracked here ros-infrastructure/rosdep#752
workaround for rosdep bug at osrf/docker_images#398

[artivis]

LGTM.