Add OCPVComputeResource entity to validate foreman_kubevirt#1402
Add OCPVComputeResource entity to validate foreman_kubevirt#1402shubhamsg199 merged 1 commit intoSatelliteQE:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
OCPVComputeResourcedocstring still refers to a Libvirt compute resource, which is misleading here and should be updated to describe the KubeVirt/OpenShift Virtualization resource. - Consider using a more appropriate field type (e.g., an integer or constrained type) for
api_portinstead of a genericStringFieldto better reflect its expected value and catch invalid input earlier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `OCPVComputeResource` docstring still refers to a Libvirt compute resource, which is misleading here and should be updated to describe the KubeVirt/OpenShift Virtualization resource.
- Consider using a more appropriate field type (e.g., an integer or constrained type) for `api_port` instead of a generic `StringField` to better reflect its expected value and catch invalid input earlier.
## Individual Comments
### Comment 1
<location> `nailgun/entities.py:1723-1724` </location>
<code_context>
ignore.add('password')
return super().read(entity, attrs, ignore, params)
+class OCPVComputeResource(AbstractComputeResource):
+ """A representation of a Libvirt Compute Resource entity."""
+
+ def __init__(self, server_config=None, **kwargs):
</code_context>
<issue_to_address>
**issue (typo):** Docstring mentions Libvirt, but this resource is configured as a Kubevirt/OpenShift Virtualization provider.
Please update the docstring to describe this as a Kubevirt/OpenShift Virtualization compute resource so it matches the default provider configuration and avoids confusion.
```suggestion
class OCPVComputeResource(AbstractComputeResource):
"""A representation of a Kubevirt/OpenShift Virtualization compute resource."""
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
556f4e3 to
f1aeb5a
Compare
f1aeb5a to
5fad61c
Compare
5fad61c to
fd81707
Compare
| 'api_port': entity_fields.StringField(required=True), | ||
| 'namespace': entity_fields.StringField(required=True), | ||
| 'token': entity_fields.StringField(required=True), | ||
| 'ca_cert': entity_fields.StringField(required=True), |
There was a problem hiding this comment.
Technically this field is not required
There was a problem hiding this comment.
Currently if we don't pass this field, we get following error
Could not create the compute resource:
SSL_connect returned=1 errno=0 state=error: certificate verify failed (self-signed certificate in certificate chain)
There was a problem hiding this comment.
Correct, but that doesn't mean it's required by the API, which is what I was after.
If your OS trusts the CA, you don't need to pass ca_cert. In most cases it won't, so you have to pass it, but that doesn't make the field required.
There was a problem hiding this comment.
ah, I get you, so technically its not required, but for our tests we don't have OS trusts the CA, so it becomes required anyway and we can revisit this in future if we cover that case
Signed-off-by: Gaurav Talreja <gtalreja@redhat.com>
fd81707 to
1252409
Compare
evgeni
left a comment
There was a problem hiding this comment.
I'd prefer that ca_cert would not be marked as required, but won't block on that.
Adding new OCPVComputeResource entity and tests to validate foreman_kubevirt as per SAT-41368