feat: adds X509 workload cert logic#1527
Conversation
andyrzhao
left a comment
There was a problem hiding this comment.
LGTM w.r.t business logic.
|
|
||
| def _read_dca_metadata_file(metadata_path): | ||
| """Loads context aware metadata from the given path. | ||
| def _read_json_file(path): |
There was a problem hiding this comment.
I think this function's name is too vague. Can you rename it to provide more context?
There was a problem hiding this comment.
renamed to load Json file, but that's not much better. Not sure what would be more descriptive, this method does simply just read a file and calls Json.load while wrapping in the appropriate error if it fails - open to suggestions
There was a problem hiding this comment.
My preference is something like _load_workload_json_file or something. The code is generic but describing when / what it is used for is useful.
Otherwise it may get shoved in a util file down the line
There was a problem hiding this comment.
With the changes in this PR, this function is used to load json configs for both secure connect as well as workload, hence the generic naming. I think it's fine as is - maybe add a function comment to clarify usage. It does seem more suitable in a generic util file, but I don't have a strong opinion here.
There was a problem hiding this comment.
added another sentence in the comment explaining the method is used for both x509 and secure connect, this definitely could be moved to a util file in the future though, just didn't think that was necessary for this PR.
Adds support for X509 workload logic that reads from the certificate_config.json file. This PR does not actually call that logic yet - hooks for actually using the new logic will come when the credential type is added.