Feature/ring 50379 publish micro app in status#13
Conversation
hervedombya
force-pushed
the
feature/RING-50379-publish-micro-app-in-status
branch
5 times, most recently
from
04cce60 to
5ddff70
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces functionality to fetch, parse, and update the UI component configuration for the micro-app upon reconciliation, and adds associated test cases. Key changes include:
- Enhancements in the reconciliation logic to fetch and apply configuration based on the state of a Deployment.
- Addition of new tests for various configuration fetch outcomes (not ready, fetch failure, parse failure, and successful fetch).
- New helper function to retrieve configuration via the Kubernetes REST client.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/controller/scalityuicomponent_controller_test.go | Added tests for configuration retrieval and condition setting; includes cleanup of associated Deployment and Service resources. |
| internal/controller/scalityuicomponent_controller.go | Integrated configuration fetching and parsing into the reconcile loop; introduced types and helper function for the micro-app configuration. |
Add Service creation and reconciliation to ScalityUIComponent controller Revert "Add Service creation and reconciliation to ScalityUIComponent controller" This reverts commit 166b7b2.
Implement mock configuration fetching and enhance ScalityUIComponent reconciliation tests Refactor ScalityUIComponent tests to remove mock client and enhance deployment/service verification Enhance ScalityUIComponent reconciliation logic and tests
hervedombya
force-pushed
the
feature/RING-50379-publish-micro-app-in-status
branch
from
c372cab to
a7d4d1b
Compare
| func (r *ScalityUIComponentReconciler) fetchMicroAppConfig(ctx context.Context, namespace, serviceName string) (string, error) { | ||
| logger := log.FromContext(ctx) | ||
|
|
||
| clientset, err := kubernetes.NewForConfig(r.Config) |
There was a problem hiding this comment.
Why not simply relying on inCluster config ?
There was a problem hiding this comment.
inCluster config would only work in the cluster and would require a special configuration for local development
There was a problem hiding this comment.
local dev is something that can be addressed by a specific config injection, when using it in prod it wil always be running in a cluster, in local it is also running in kind so in a cluster, expect when you attach it to a cluster. But anyway with below comment I think we don't need this client here
| restClient := clientset.CoreV1().RESTClient() | ||
| req := restClient.Get(). | ||
| Namespace(namespace). | ||
| Resource("services"). | ||
| Name(fmt.Sprintf("%s:%d", serviceName, DefaultServicePort)). |
There was a problem hiding this comment.
Isn't it us to create the service ? SO i think we already know the service name and don't need to fetch it right ?
There was a problem hiding this comment.
I mean we can dircetly query serviceName + '.' + namespace + '.svc.cluster.local'
There was a problem hiding this comment.
with this approach, we need to implement a full HTTP client, right ?
There was a problem hiding this comment.
What do you mean by a full http client.
It's just http.NewRequest("GET", "http://"+ serviceName + '.' + namespace + '.svc.cluster.local/.well-known/micro-app-configuration", nil)
No description provided.