[patch] create functions to fetch cluster issuer#161
[patch] create functions to fetch cluster issuer#161
Conversation
durera
left a comment
There was a problem hiding this comment.
Can we also add unittests for the new function to the tests. Don't need you to fill existing test gaps, but we don't want to create more gaps going forward. Use mock framework, and include good/bad path test scenarios.
Also note that delivery is tied to ibm-mas/cli#1983 (review)
We need to update the CLI PR to include the RBAC additions for the new capability that will be used by the CLI user:
https://github.com/ibm-mas/cli/tree/master/rbac/install/user .. the clusterrole will need to be updated to provide the necessary access for the ClusterIssuer resource before this pair of PRs can be merged
src/mas/devops/ocp.py
Outdated
| return clusterIssuers | ||
|
|
||
|
|
||
| def getClusterIssuer(dynClient: DynamicClient, name: str) -> str: |
There was a problem hiding this comment.
The typehint is incorrect, this is not returning a string clusterIssuer is the object returned by the clusterIssuerAPI, rather than the name of the issuer.
There was a problem hiding this comment.
In other functions, it's returning a dict. Should we use dict as convention to all or the actual ResourceInstance ?

python-devops/src/mas/devops/ocp.py
Lines 339 to 358 in 8749a84
| ClusterIssuer: The ClusterIssuer resource, or None if not found | ||
|
|
||
| Raises: | ||
| NotFoundError: If the ClusterIssuer does not exist (caught and returns None) |
There was a problem hiding this comment.
We are catching a NotFoundError and returning None, so we shouldn't document that this raises NotFoundError ... the documentation should only list exceptions that can be raised by calling the function, not exceptions handling internally within the function.
There was a problem hiding this comment.
Okay, I agree with that. But there are several example in the code that handle the exceptions like this. Fya:
python-devops/src/mas/devops/ocp.py
Lines 416 to 437 in 8749a84
python-devops/src/mas/devops/ocp.py
Lines 339 to 358 in 8749a84
I just followed the standard of the code. So, should we change all or keep as is ?
Description
getClusterIssuerandgetClusterIssuersto support [minor] Support BYO ClusterIssuer cli#1983.