context: various cleanups and improvements

[thaJeztah]

cli/context/store: metadataStore.get(), .remove(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally.

cli/context/store: listRecursivelyMetadataDirs(): use filepath.Join()

Looks like the intent here is to get the path of a subdirectory.

cli/context/store: removeAllContextData(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally. This also fixes a test which incorrectly used "names" as
pseudo-IDs.

cli/context/store: TestTlsCreateUpdateGetRemove(): use correct ID

This test was depending on the fact that contextDir's are a string,
and for the test is was using the context name as a pseudo-ID.

This patch updates the test to be more explicit where ID's and where
names are used.

cli/context/store: removeAllEndpointData(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally.

cli/context/store: createOrUpdate(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally.

cli/context/store: getData(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally.

cli/context/store: remove(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally.

cli/context/store: listContextData(): accept name instead of ID

This allows callers to just pass the name, and handle the conversion to ID and
path internally.

cli/context/store: remove endpointDir(), filePath()

These were just to produce the full path of subdirectories within the context
directory; removing the extra abstraction.

cli/context/store: simplify error handling, and make it more idiomatic

The package defined various special errors; these errors existed for two reasons;

• being able to distinguish "not found" errors from other errors (as "not found"
  errors can be ignored in various cases).
• to be able to update the context name in the error message after the error
  was created. This was needed in cases where the name was not available at the
  location where the error was produced (e.g. only the "id" was present), and
  the helpers to detect "not found" errors did not support wrapped errors (so
  wrapping the error with a "name" could break logic); a setContextName interface
  and corresponding patchErrContextName() utility was created for this (which
  was a "creative", but not very standard approach).

This patch:

• Removes the special error-types, replacing them with errdefs definitions (which
  is a more common approach in our code-base to detect error types / classes).
• Removes the internal utilities for error-handling, and deprecates the exported
  utilities (to allow external consumers to adjust their code).
• Some errors have been enriched with detailed information (which may be useful
  for debugging / problem solving).
• Note that in some cases, patchErrContextName() was called, but the code
  producing the error would never return a setContextName error, so would
  never update the error message.

cli/context/store: New(): return concrete type

Go conventions are for interfaces to be defined on the receiver side,
and for producers to return concrete types. This patch changes the
constructor to return a concrete type.

cli/context/store: List(): don't interrupt listing for not-found errors

There's no reason to stop listing contexts if a context does not exist
while iterating over the directories,

[codecov-commenter]

Codecov Report

Merging #3790 (cd7c493) into master (a496a7d) will decrease coverage by 0.06%.
The diff coverage is 57.29%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3790      +/-   ##
==========================================
- Coverage   59.29%   59.22%   -0.07%     
==========================================
  Files         288      288              
  Lines       24641    24616      -25     
==========================================
- Hits        14610    14578      -32     
- Misses       9161     9163       +2     
- Partials      870      875       +5     

[vvoland]

I'm slightly hesitant on removing these. IMO there's some value in having these helper functions due to the signature prompting you on what you should supply to obtain the final directory.
If I would be writing a new code and need to obtain the file path, then I would have to search for other code that does this to know how to construct the whole path with filepath.Join.
With those conversion functions I just need to know that there is this function and then the arguments to the functions will guide me on what I need to provide. For example if I miss the endpointName then the compiler will yell at me, whereas if I forget it in filepath.Join then I won't know that at the compile time.
WDYT?

[thaJeztah]

Let me have a look. My main reasons were that;

• Each of them would be reconstructing the parent directory, so I initially wanted to re-use contextDir() and endpointDir()
• The fileDir() one felt really redundant
• The endpointDir() as used in listContextData() was really silly, as we already were traversing the directory, and then were re-constructing the name … to get the name of the directory we were in.

Perhaps I can bring back endpointDir(), and only remove filePath(); let me see how that looks.

There were some other changes I was considering;

• As the contextDir depends on the context store, I want to look at passing that directory in (effectively, calculating that path should be something done by the context store)
• Given that for the TLS-store we only want ca.pem, cert.pem and key.pem, perhaps add accessors for those instead of trying to keep it “too generic”

A lot of the code was written to be really “generic”, but that’s not how it’s used, and perhaps we should rewrite more code to be specific to how it’s actually used.

[vvoland]

Makes sense to me, thanks for the explanation! 👍

[thaJeztah]

@vvoland I updated, and restored the getEndpointDir(), so only removing the filePath()