-
Notifications
You must be signed in to change notification settings - Fork 122
CSPL-3675 Update Operator-SDK to v1.39 #1488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pull Request Test Coverage Report for Build 15969470039Details
💛 - Coveralls |
…L-3675-operator-sdk # Conflicts: # .biased_lang_exclude
# Conflicts: # .env # go.mod
cmd/main.go
Outdated
@@ -123,56 +125,56 @@ func main() { | |||
os.Exit(1) | |||
} | |||
|
|||
if err = (&controllers.ClusterMasterReconciler{ | |||
if err = (&controller2.ClusterMasterReconciler{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to splunk-controller instead.
pkg/splunk/controller/controller.go
Outdated
err = ctrl.Watch( | ||
source.Kind(mgr.GetCache(), instance, &handler.TypedEnqueueRequestForObject[splcommon.MetaObject]{}), | ||
) | ||
//err = ctrl.Watch(&source.Kind{Type: instance}, &handler.EnqueueRequestForObject{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented line if not needed.
@@ -5,6 +5,7 @@ on: | |||
- develop | |||
- main | |||
- feature** | |||
- CSPL-3675-operator-sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remember to remove it once verified.
handler.EnqueueRequestForOwner( | ||
mgr.GetScheme(), | ||
mgr.GetRESTMapper(), | ||
&appsv1.StatefulSet{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include handler.OnlyControllerOwner() here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one im not sure about 100% @vivekr-splunk Could You take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed for now but if we want to split the operator in the future we should probably take a look at that
return !e.DeleteStateUnknown | ||
}, | ||
} | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly return without declaring a var err, Same below.
if !stringInSlice(newObj.Name, []string{"clustermasters.enterprise.splunk.com", | ||
// Process only specific CRD names | ||
if !stringInSlice(newObj.Name, []string{ | ||
"clustermasters.enterprise.splunk.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about clustermanagers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't there before, should it even be there? @vivekr-splunk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove branch from integ tests pipeline on merge
2a20504
to
aabe029
Compare
e813abe
to
6547244
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades the Operator SDK to v1.39 and updates related APIs, generated code, and CRDs to match new SDK requirements.
- Update RESTClientForGVK calls to include an
http.Client
parameter - Replace deprecated
MetricsBindAddress
with the newserver.Options
API - Switch from
ResourceRequirements
toVolumeResourceRequirements
for PVCs (requires verification) - Rename the
controller
package imports tosplkcontroller
across code and tests - Bump
go.mod
and regenerate CRDs withx-kubernetes-list-type: atomic
and additional fields
Reviewed Changes
Copilot reviewed 91 out of 98 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/testenv/verificationutils.go | Added stabilization sleep and StabilizationDuration constant (missing import) |
test/testenv/util.go | Introduced DumpGetSecrets and changed PVC resources type |
pkg/splunk/enterprise/configuration.go | Changed PVC resources type |
test/testenv/testenv.go | Updated manager Metrics options |
go.mod | Bumped Go version and updated SDK and dependency versions |
Comments suppressed due to low confidence (4)
test/testenv/util.go:740
- [nitpick] The comment says "list of pods" but this function is fetching secrets. Update the comment to accurately describe that it lists secrets.
// DumpGetSecrets prints and returns list of pods in the namespace
test/testenv/verificationutils.go:36
- The
time
package is not imported, causing a compile error. Addimport "time"
at the top of the file.
var StabilizationDuration = time.Second * 20
test/testenv/util.go:527
- There is no
corev1.VolumeResourceRequirements
type in Kubernetes API; it should remaincorev1.ResourceRequirements
to define PVC resource requests.
Resources: corev1.VolumeResourceRequirements{
pkg/splunk/enterprise/configuration.go:142
- Incorrect use of
corev1.VolumeResourceRequirements
—this type does not exist. Usecorev1.ResourceRequirements
for PVC specs.
Resources: corev1.VolumeResourceRequirements{
test/testenv/util.go
Outdated
|
||
output, err := exec.Command("kubectl", "get", "secrets", "-n", ns).Output() | ||
if err != nil { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning nil
on error may lead to nil-slice operations downstream. Consider returning an empty slice or logging the error before returning.
return nil | |
logf.Log.Error(err, "Failed to execute command", "command", "kubectl get secrets -n "+ns) | |
return []string{} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
v1.33 migration
v1.34 migration
v1.36 migration
v1.38 migration
v1.39 migration
Changes related to the OperatorSDK upgrade:
predicate.GenerationChangedPredicate{}
andpredicate.AnnotationChangedPredicate{},
for predicateOther changes
SPLUNK_ENTERPRISE_IMAGE
var value