feat: Add App CRD on MVP branch#1826
Conversation
|
There are some merge conflicts, can we rebase and push again |
naimulh247
left a comment
naimulh247
left a comment
There was a problem hiding this comment.
we might want to consider using pointers as part of the app_types.go to know if user defined the field or not
api/apps/v1alpha1/app_types.go
Outdated
| const ( | ||
| // AppPausedAnnotation is the annotation that pauses the reconciliation (triggers | ||
| // an immediate requeue) | ||
| AppPausedAnnotation = "app.enterprise.splunk.com/paused" |
There was a problem hiding this comment.
its not part of the enterprise group anymore :)
There was a problem hiding this comment.
Thank you for catching that! Changed that to apps.splunk.com/paused
Signed-off-by: jzywieck <jzywiecki@splunk.com>
jzywiecki
force-pushed
the
mvp/app-framework-v1-app-crd
branch
from
bffa2f9 to
86350ea
Compare
Signed-off-by: jzywieck <jzywiecki@splunk.com>
vivekr-splunk
left a comment
vivekr-splunk
left a comment
There was a problem hiding this comment.
I found two blocking issues in the current App CRD/controller shape.
api/apps/v1alpha1/app_types.go
Outdated
| TargetRef AppTargetRef `json:"targetRef"` | ||
|
|
||
| // +kubebuilder:validation:Required | ||
| SourceRef AppSource `json:"sourceRef"` |
There was a problem hiding this comment.
I think sourceRef needs to be a reference type rather than an embedded AppSource resource. In the generated schema and sample, each App now carries an entire AppSource object inline, which bypasses the separate AppSource CRD/controller/status and makes it hard to share one source across multiple apps. From the operator product point of view, that looks like the wrong API contract to publish.
There was a problem hiding this comment.
Thats right, instead of embedding I have aligned it with ERD and created referencing of it
| // | ||
| // For more details, check Reconcile and its Result here: | ||
| // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.21.0/pkg/reconcile | ||
| func (r *AppReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { |
There was a problem hiding this comment.
This looks user-visible too early to me. cmd/main.go wires the App controller into the manager, but Reconcile still returns immediately without reading the App, updating status, or driving any app-install behavior. As merged, we would expose an App CRD that appears supported but does nothing once created, which does not seem safe from the operator product point of view.
There was a problem hiding this comment.
I think it should be fine for now, at the moment we are just figuring out what the CRD / user fields we need... the reconciliation working would be a different MR
There was a problem hiding this comment.
Hey @vivekr-splunk, this changes are not planned to be merged to the main branch yet, we are working on a separate app-deployment branch and its one of the first steps to getting closer to creating this CRD (its part of a M1). The controller implementation is part of the further steps, for now the changes you are refering to are the stub implementation from operator-sdk
Signed-off-by: jzywieck <jzywiecki@splunk.com>
jzywiecki
force-pushed
the
mvp/app-framework-v1-app-crd
branch
from
99d10eb to
f5518fe
Compare
Signed-off-by: jzywieck <jzywiecki@splunk.com>
3 participants
Description
This PR adds the new
Appcustom resource definition to the operator.Key Changes
AppAPI type underenterprise.splunk.com/v4AppCRDAppmanifest and wired it into sample kustomizationTesting and Verification
make manifests generateRelated Issues
Screenshots
PR Checklist