adds code commentary and improves a log message #52
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mhrivnak The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @mhrivnak! It looks like this is your first PR to openshift-assisted/cluster-api-agent 🎉 |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @mhrivnak. Thanks for your PR. I'm waiting for a openshift-assisted member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
size/S
| @@ -87,6 +87,7 @@ type AgentBootstrapConfigStatus struct { | |||
| // AgentRef references the agent this agent bootstrap config has booted | |||
| AgentRef *corev1.LocalObjectReference `json:"agentRef,omitempty"` | |||
|
|
|||
| // TODO: should this be on the spec? | |||
There was a problem hiding this comment.
I believe we initially moved it here to prevent the user from setting this field.
What are the reasons to move it to spec?
EDIT: Nvm, I see your comment below about it being unconventional to set status on a resource it doesn't own
There was a problem hiding this comment.
It's desired state. We want the URL to be used in the bootstrap process. And of course in a declarative API, we "spec"ify our desired state in the spec. :)
The status should be reserved for observations about the world that were taken the last time the owning controller reconciled this resource.
There was a problem hiding this comment.
Ah I see! That makes sense, thank you for the explanation! CC @rccrdpccl
There was a problem hiding this comment.
Good point, however I was thinking that might be a better idea to remove the InfraEnv controller altogether and just watch InfraEnv from the ABC controller, so we might remove the field altogether.
I also think we should probably apply the same concept to other controller we have for assisted's resources. WDYT?
| // TODO maybe we can find a better way than listing and scanning every BMH | ||
| // that exists, such as explicitly watching BMHs and setting up a | ||
| // FieldIndexer. | ||
| // Example: https://github.com/kubernetes-sigs/kubebuilder/blob/master/docs/book/src/reference/watching-resources/testdata/external-indexed-field/controller.go |
There was a problem hiding this comment.
Good idea! Will look into this, thank you!!
| @@ -432,12 +434,14 @@ func (r *AgentBootstrapConfigReconciler) setMetal3MachineImage( | |||
| return err | |||
| } | |||
| log.V(logutil.TraceLevel). | |||
| Info("Added ISO URLs to metal3 machines", "machine", metal3Machine.Name, "namespace", metal3Machine.Namespace) | |||
| Info("Added ISO URL to metal3 machine", "machine", metal3Machine.Name, "namespace", metal3Machine.Namespace) | |||
There was a problem hiding this comment.
Good catch, thank you!!
| @@ -553,6 +559,11 @@ func (r *AgentBootstrapConfigReconciler) SetupWithManager(mgr ctrl.Manager) erro | |||
| ). | |||
| Watches( | |||
| &hivev1.ClusterDeployment{}, | |||
| // TODO does this depend on the name and namespace for the | |||
| // ClusterDeployment being the same as the name and namespace of the | |||
| // ABC? Could we put an owner reference on the CD instead and use | |||
There was a problem hiding this comment.
Yes we definitely need to change a few naming convenction, ClusterImageSet is another example.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
No description provided.