From 3dc472c6c29041fb1ab7c5119b0b2e6a1c7bf500 Mon Sep 17 00:00:00 2001 From: srinivasr Date: Mon, 24 Aug 2026 15:01:30 +0530 Subject: [PATCH 1/2] Fix copylocks by using pointer receivers for protobuf models Generated protobuf structs such as ApplicationSyncState embed protoimpl.MessageState, which contains a sync.Mutex. Their methods used value receivers, so the lock state was copied on every call or pass, which go vet flags as a copylock violation. Switch the methods on ApplicationSyncState and ApplicationLiveStateVersion to pointer receivers and update all caller sites in driftdetector, livestatestore, livestatereporter, planpreview, and applicationlivestatestore to pass pointers. Convert range loops in the affected model tests to index loops so test cases are not copied. Signed-off-by: srinivasr --- .../piped/driftdetector/cloudrun/detector.go | 8 +-- pkg/app/piped/driftdetector/detector.go | 8 +-- pkg/app/piped/driftdetector/ecs/detector.go | 10 ++-- .../driftdetector/kubernetes/detector.go | 8 +-- .../piped/driftdetector/lambda/detector.go | 8 +-- .../piped/driftdetector/terraform/detector.go | 4 +- .../livestatereporter/kubernetes/reporter.go | 10 ++-- .../livestatestore/kubernetes/appnodes.go | 52 ++++++++++++------- .../livestatestore/kubernetes/kubernetes.go | 4 +- .../piped/livestatestore/kubernetes/store.go | 20 +++---- pkg/app/piped/planpreview/builder.go | 4 +- .../platformprovider/kubernetes/state.go | 4 +- pkg/app/pipedv1/planpreview/builder.go | 4 +- .../server/applicationlivestatestore/store.go | 2 +- pkg/model/application.go | 6 +-- pkg/model/application_live_state.go | 4 +- pkg/model/application_live_state_test.go | 5 +- pkg/model/application_test.go | 8 +-- pkg/model/deployment_chain_test.go | 4 +- pkg/model/deployment_test.go | 6 ++- pkg/model/piped_test.go | 10 ++-- pkg/model/planpreview.go | 2 +- pkg/model/project_test.go | 3 +- 23 files changed, 109 insertions(+), 85 deletions(-) diff --git a/pkg/app/piped/driftdetector/cloudrun/detector.go b/pkg/app/piped/driftdetector/cloudrun/detector.go index 034332bb49..0910dcd23c 100644 --- a/pkg/app/piped/driftdetector/cloudrun/detector.go +++ b/pkg/app/piped/driftdetector/cloudrun/detector.go @@ -47,7 +47,7 @@ type secretDecrypter interface { } type reporter interface { - ReportApplicationSyncState(ctx context.Context, appID string, state model.ApplicationSyncState) error + ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error } type Detector interface { @@ -312,9 +312,9 @@ func (d *detector) loadApplicationConfiguration(repoPath string, app *model.Appl return cfg, nil } -func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncState { +func makeSyncState(r *provider.DiffResult, commit string) *model.ApplicationSyncState { if r.NoChange() { - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_SYNCED, Timestamp: time.Now().Unix(), } @@ -337,7 +337,7 @@ func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncS }) b.WriteString(details) - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_OUT_OF_SYNC, ShortReason: shortReason, Reason: b.String(), diff --git a/pkg/app/piped/driftdetector/detector.go b/pkg/app/piped/driftdetector/detector.go index 358e8a43df..35e3a3a877 100644 --- a/pkg/app/piped/driftdetector/detector.go +++ b/pkg/app/piped/driftdetector/detector.go @@ -67,7 +67,7 @@ type Detector interface { type detector struct { apiClient apiClient detectors []providerDetector - syncStates map[string]model.ApplicationSyncState + syncStates map[string]*model.ApplicationSyncState mu sync.RWMutex logger *zap.Logger } @@ -91,7 +91,7 @@ func NewDetector( d := &detector{ apiClient: apiClient, detectors: make([]providerDetector, 0, len(cfg.PlatformProviders)), - syncStates: make(map[string]model.ApplicationSyncState), + syncStates: make(map[string]*model.ApplicationSyncState), logger: logger.Named("drift-detector"), } @@ -219,7 +219,7 @@ func (d *detector) Run(ctx context.Context) error { return nil } -func (d *detector) ReportApplicationSyncState(ctx context.Context, appID string, state model.ApplicationSyncState) error { +func (d *detector) ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error { d.mu.RLock() curState, ok := d.syncStates[appID] d.mu.RUnlock() @@ -230,7 +230,7 @@ func (d *detector) ReportApplicationSyncState(ctx context.Context, appID string, _, err := d.apiClient.ReportApplicationSyncState(ctx, &pipedservice.ReportApplicationSyncStateRequest{ ApplicationId: appID, - State: &state, + State: state, }) if err != nil { d.logger.Error("failed to report application sync state", diff --git a/pkg/app/piped/driftdetector/ecs/detector.go b/pkg/app/piped/driftdetector/ecs/detector.go index 1fa9287755..b85c970d5e 100644 --- a/pkg/app/piped/driftdetector/ecs/detector.go +++ b/pkg/app/piped/driftdetector/ecs/detector.go @@ -51,7 +51,7 @@ type secretDecrypter interface { } type reporter interface { - ReportApplicationSyncState(ctx context.Context, appID string, state model.ApplicationSyncState) error + ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error } type Detector interface { @@ -445,16 +445,16 @@ func (d *detector) loadApplicationConfiguration(repoPath string, app *model.Appl return cfg, nil } -func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncState { +func makeSyncState(r *provider.DiffResult, commit string) *model.ApplicationSyncState { if r.NoChange() { - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_SYNCED, Timestamp: time.Now().Unix(), } } if ignoreAutoScalingDiff(r) { - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_SYNCED, ShortReason: "Ignore diff of `desiredCount`.", Reason: "`desiredCount` is 0 or not defined in your config (which means ignoring updating desiredCount) and only `desiredCount` is changed.", @@ -479,7 +479,7 @@ func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncS }) b.WriteString(details) - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_OUT_OF_SYNC, ShortReason: shortReason, Reason: b.String(), diff --git a/pkg/app/piped/driftdetector/kubernetes/detector.go b/pkg/app/piped/driftdetector/kubernetes/detector.go index 2092ed8d91..b8ebd7c8d4 100644 --- a/pkg/app/piped/driftdetector/kubernetes/detector.go +++ b/pkg/app/piped/driftdetector/kubernetes/detector.go @@ -47,7 +47,7 @@ type secretDecrypter interface { } type reporter interface { - ReportApplicationSyncState(ctx context.Context, appID string, state model.ApplicationSyncState) error + ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error } type Detector interface { @@ -382,9 +382,9 @@ func filterIgnoringManifests(manifests []provider.Manifest) []provider.Manifest return out } -func makeSyncState(r *provider.DiffListResult, commit string) model.ApplicationSyncState { +func makeSyncState(r *provider.DiffListResult, commit string) *model.ApplicationSyncState { if r.NoChange() { - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_SYNCED, ShortReason: "", Reason: "", @@ -413,7 +413,7 @@ func makeSyncState(r *provider.DiffListResult, commit string) model.ApplicationS }) b.WriteString(details) - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_OUT_OF_SYNC, ShortReason: shortReason, Reason: b.String(), diff --git a/pkg/app/piped/driftdetector/lambda/detector.go b/pkg/app/piped/driftdetector/lambda/detector.go index 1e56c52853..b722c57e6a 100644 --- a/pkg/app/piped/driftdetector/lambda/detector.go +++ b/pkg/app/piped/driftdetector/lambda/detector.go @@ -49,7 +49,7 @@ type secretDecrypter interface { } type reporter interface { - ReportApplicationSyncState(ctx context.Context, appID string, state model.ApplicationSyncState) error + ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error } type Detector interface { @@ -360,9 +360,9 @@ func (d *detector) loadApplicationConfiguration(repoPath string, app *model.Appl return cfg, nil } -func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncState { +func makeSyncState(r *provider.DiffResult, commit string) *model.ApplicationSyncState { if r.NoChange() { - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_SYNCED, Timestamp: time.Now().Unix(), } @@ -385,7 +385,7 @@ func makeSyncState(r *provider.DiffResult, commit string) model.ApplicationSyncS }) b.WriteString(details) - return model.ApplicationSyncState{ + return &model.ApplicationSyncState{ Status: model.ApplicationSyncStatus_OUT_OF_SYNC, ShortReason: shortReason, Reason: b.String(), diff --git a/pkg/app/piped/driftdetector/terraform/detector.go b/pkg/app/piped/driftdetector/terraform/detector.go index 895fbfef6e..eb7a8722b8 100644 --- a/pkg/app/piped/driftdetector/terraform/detector.go +++ b/pkg/app/piped/driftdetector/terraform/detector.go @@ -48,7 +48,7 @@ type secretDecrypter interface { } type reporter interface { - ReportApplicationSyncState(ctx context.Context, appID string, state model.ApplicationSyncState) error + ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error } type Detector interface { @@ -281,7 +281,7 @@ func (d *detector) checkApplication(ctx context.Context, app *model.Application, return err } - return d.reporter.ReportApplicationSyncState(ctx, app.Id, *state) + return d.reporter.ReportApplicationSyncState(ctx, app.Id, state) } func makeSyncState(r provider.PlanResult, commit string) (*model.ApplicationSyncState, error) { diff --git a/pkg/app/piped/livestatereporter/kubernetes/reporter.go b/pkg/app/piped/livestatereporter/kubernetes/reporter.go index 928c8499aa..ca5b4bd8dc 100644 --- a/pkg/app/piped/livestatereporter/kubernetes/reporter.go +++ b/pkg/app/piped/livestatereporter/kubernetes/reporter.go @@ -56,7 +56,7 @@ type reporter struct { snapshotFlushInterval time.Duration logger *zap.Logger - snapshotVersions map[string]model.ApplicationLiveStateVersion + snapshotVersions map[string]*model.ApplicationLiveStateVersion } func NewReporter(cp config.PipedPlatformProvider, appLister applicationLister, stateGetter kubernetes.Getter, apiClient apiClient, logger *zap.Logger) Reporter { @@ -72,7 +72,7 @@ func NewReporter(cp config.PipedPlatformProvider, appLister applicationLister, s flushInterval: 5 * time.Second, snapshotFlushInterval: 10 * time.Minute, logger: logger, - snapshotVersions: make(map[string]model.ApplicationLiveStateVersion), + snapshotVersions: make(map[string]*model.ApplicationLiveStateVersion), } } @@ -128,7 +128,7 @@ func (r *reporter) flushSnapshots(ctx context.Context) { Kubernetes: &model.KubernetesApplicationLiveState{ Resources: state.Resources, }, - Version: &state.Version, + Version: state.Version, } snapshot.DetermineAppHealthStatus() req := &pipedservice.ReportApplicationLiveStateRequest{ @@ -154,12 +154,12 @@ func (r *reporter) flushEvents(ctx context.Context) error { } filteredEvents := make([]*model.KubernetesResourceStateEvent, 0, len(events)) - for i, event := range events { + for _, event := range events { snapshotVersion, ok := r.snapshotVersions[event.ApplicationId] if ok && event.SnapshotVersion.IsBefore(snapshotVersion) { continue } - filteredEvents = append(filteredEvents, &events[i]) + filteredEvents = append(filteredEvents, event) } if len(filteredEvents) == 0 { return nil diff --git a/pkg/app/piped/livestatestore/kubernetes/appnodes.go b/pkg/app/piped/livestatestore/kubernetes/appnodes.go index 646d25daba..e7632b8a70 100644 --- a/pkg/app/piped/livestatestore/kubernetes/appnodes.go +++ b/pkg/app/piped/livestatestore/kubernetes/appnodes.go @@ -29,7 +29,7 @@ type appNodes struct { appID string managingNodes map[string]node dependedNodes map[string]node - version model.ApplicationLiveStateVersion + version *model.ApplicationLiveStateVersion mu sync.RWMutex } @@ -39,7 +39,7 @@ type node struct { appID string key provider.ResourceKey unstructured *unstructured.Unstructured - state model.KubernetesResourceState + state *model.KubernetesResourceState } func (n node) Manifest() provider.Manifest { @@ -64,7 +64,10 @@ func (a *appNodes) addManagingResource(uid string, key provider.ResourceKey, obj a.mu.Lock() oriNode, hasOriNode := a.managingNodes[uid] - version := a.version + version := &model.ApplicationLiveStateVersion{ + Timestamp: a.version.Timestamp, + Index: a.version.Index, + } a.managingNodes[uid] = n a.updateVersion(now) a.mu.Unlock() @@ -78,8 +81,8 @@ func (a *appNodes) addManagingResource(uid string, key provider.ResourceKey, obj Id: uuid.New().String(), ApplicationId: a.appID, Type: model.KubernetesResourceStateEvent_ADD_OR_UPDATED, - State: &n.state, - SnapshotVersion: &version, + State: n.state, + SnapshotVersion: version, CreatedAt: now.Unix(), }, true } @@ -92,7 +95,10 @@ func (a *appNodes) deleteManagingResource(uid string, _ provider.ResourceKey, no return model.KubernetesResourceStateEvent{}, false } - version := a.version + version := &model.ApplicationLiveStateVersion{ + Timestamp: a.version.Timestamp, + Index: a.version.Index, + } delete(a.managingNodes, uid) a.updateVersion(now) a.mu.Unlock() @@ -101,8 +107,8 @@ func (a *appNodes) deleteManagingResource(uid string, _ provider.ResourceKey, no Id: uuid.New().String(), ApplicationId: a.appID, Type: model.KubernetesResourceStateEvent_DELETED, - State: &n.state, - SnapshotVersion: &version, + State: n.state, + SnapshotVersion: version, CreatedAt: now.Unix(), }, true } @@ -118,7 +124,10 @@ func (a *appNodes) addDependedResource(uid string, key provider.ResourceKey, obj a.mu.Lock() oriNode, hasOriNode := a.dependedNodes[uid] - version := a.version + version := &model.ApplicationLiveStateVersion{ + Timestamp: a.version.Timestamp, + Index: a.version.Index, + } a.dependedNodes[uid] = n a.updateVersion(now) a.mu.Unlock() @@ -132,8 +141,8 @@ func (a *appNodes) addDependedResource(uid string, key provider.ResourceKey, obj Id: uuid.New().String(), ApplicationId: a.appID, Type: model.KubernetesResourceStateEvent_ADD_OR_UPDATED, - State: &n.state, - SnapshotVersion: &version, + State: n.state, + SnapshotVersion: version, CreatedAt: now.Unix(), }, true } @@ -146,7 +155,10 @@ func (a *appNodes) deleteDependedResource(uid string, _ provider.ResourceKey, no return model.KubernetesResourceStateEvent{}, false } - version := a.version + version := &model.ApplicationLiveStateVersion{ + Timestamp: a.version.Timestamp, + Index: a.version.Index, + } delete(a.dependedNodes, uid) a.updateVersion(now) a.mu.Unlock() @@ -155,8 +167,8 @@ func (a *appNodes) deleteDependedResource(uid string, _ provider.ResourceKey, no Id: uuid.New().String(), ApplicationId: a.appID, Type: model.KubernetesResourceStateEvent_DELETED, - State: &n.state, - SnapshotVersion: &version, + State: n.state, + SnapshotVersion: version, CreatedAt: now.Unix(), }, true } @@ -168,13 +180,17 @@ func (a *appNodes) getManagingNodes() map[string]node { return a.managingNodes } -func (a *appNodes) getNodes() (map[string]node, model.ApplicationLiveStateVersion) { +func (a *appNodes) getNodes() (map[string]node, *model.ApplicationLiveStateVersion) { a.mu.RLock() defer a.mu.RUnlock() var ( - version = a.version - nodes = make(map[string]node, len(a.managingNodes)+len(a.dependedNodes)) + // return a value copy so the caller can't mutate a.version outside the lock + version = model.ApplicationLiveStateVersion{ + Timestamp: a.version.Timestamp, + Index: a.version.Index, + } + nodes = make(map[string]node, len(a.managingNodes)+len(a.dependedNodes)) ) for k, n := range a.dependedNodes { nodes[k] = n @@ -182,7 +198,7 @@ func (a *appNodes) getNodes() (map[string]node, model.ApplicationLiveStateVersio for k, n := range a.managingNodes { nodes[k] = n } - return nodes, version + return nodes, &version } func (a *appNodes) updateVersion(now time.Time) { diff --git a/pkg/app/piped/livestatestore/kubernetes/kubernetes.go b/pkg/app/piped/livestatestore/kubernetes/kubernetes.go index 83e2425fa9..430bf1b4ab 100644 --- a/pkg/app/piped/livestatestore/kubernetes/kubernetes.go +++ b/pkg/app/piped/livestatestore/kubernetes/kubernetes.go @@ -52,7 +52,7 @@ type Getter interface { type AppState struct { Resources []*model.KubernetesResourceState - Version model.ApplicationLiveStateVersion + Version *model.ApplicationLiveStateVersion } type EventIterator struct { @@ -60,7 +60,7 @@ type EventIterator struct { store *store } -func (it EventIterator) Next(maxNum int) []model.KubernetesResourceStateEvent { +func (it EventIterator) Next(maxNum int) []*model.KubernetesResourceStateEvent { return it.store.nextEvents(it.id, maxNum) } diff --git a/pkg/app/piped/livestatestore/kubernetes/store.go b/pkg/app/piped/livestatestore/kubernetes/store.go index 721af0cfe5..1ef5ca7cf0 100644 --- a/pkg/app/piped/livestatestore/kubernetes/store.go +++ b/pkg/app/piped/livestatestore/kubernetes/store.go @@ -42,7 +42,7 @@ type store struct { resources map[string]appResource mu sync.RWMutex - events []model.KubernetesResourceStateEvent + events []*model.KubernetesResourceStateEvent iterators map[int]int nextIteratorID int eventMu sync.Mutex @@ -120,8 +120,9 @@ func (s *store) addResource(obj *unstructured.Unstructured, appID string) { appID: appID, managingNodes: make(map[string]node), dependedNodes: make(map[string]node), - version: model.ApplicationLiveStateVersion{ + version: &model.ApplicationLiveStateVersion{ Timestamp: now.Unix(), + Index: 0, }, } s.apps[appID] = app @@ -130,7 +131,7 @@ func (s *store) addResource(obj *unstructured.Unstructured, appID string) { // Append the resource to the application's managingNodes. if event, ok := app.addManagingResource(uid, key, obj, now); ok { - s.addEvent(event) + s.addEvent(&event) } // And update the resources. @@ -154,7 +155,7 @@ func (s *store) addResource(obj *unstructured.Unstructured, appID string) { s.mu.RUnlock() if ok { if event, ok := app.addDependedResource(uid, key, obj, now); ok { - s.addEvent(event) + s.addEvent(&event) } } } @@ -206,7 +207,7 @@ func (s *store) onDeleteResource(obj *unstructured.Unstructured) { s.mu.RUnlock() if ok { if event, ok := app.deleteManagingResource(uid, key, now); ok { - s.addEvent(event) + s.addEvent(&event) } } return @@ -240,7 +241,7 @@ func (s *store) onDeleteResource(obj *unstructured.Unstructured) { s.mu.RUnlock() if ok { if event, ok := app.deleteDependedResource(uid, key, now); ok { - s.addEvent(event) + s.addEvent(&event) } } } @@ -289,8 +290,7 @@ func (s *store) getAppLiveState(appID string) (AppState, bool) { resources = make([]*model.KubernetesResourceState, 0, len(nodes)) ) for i := range nodes { - state := nodes[i].state - resources = append(resources, &state) + resources = append(resources, nodes[i].state) } return AppState{ @@ -315,7 +315,7 @@ func (s *store) GetAppLiveManifests(appID string) []provider.Manifest { return manifests } -func (s *store) addEvent(event model.KubernetesResourceStateEvent) { +func (s *store) addEvent(event *model.KubernetesResourceStateEvent) { s.eventMu.Lock() defer s.eventMu.Unlock() @@ -328,7 +328,7 @@ func (s *store) addEvent(event model.KubernetesResourceStateEvent) { s.removeOldEvents(num) } -func (s *store) nextEvents(iteratorID, maxNum int) []model.KubernetesResourceStateEvent { +func (s *store) nextEvents(iteratorID, maxNum int) []*model.KubernetesResourceStateEvent { s.eventMu.Lock() defer s.eventMu.Unlock() diff --git a/pkg/app/piped/planpreview/builder.go b/pkg/app/piped/planpreview/builder.go index a36776a060..4ca186739b 100644 --- a/pkg/app/piped/planpreview/builder.go +++ b/pkg/app/piped/planpreview/builder.go @@ -212,7 +212,7 @@ func (b *builder) buildApp(ctx context.Context, worker int, command string, app logger.Info("will decide sync strategy for an application") - r := model.MakeApplicationPlanPreviewResult(*app) + r := model.MakeApplicationPlanPreviewResult(app) var preCommit string // Find the commit of the last successful deployment. @@ -321,7 +321,7 @@ func (b *builder) findTriggerApps(ctx context.Context, repo git.Repo, apps []*mo continue } - r := model.MakeApplicationPlanPreviewResult(*app) + r := model.MakeApplicationPlanPreviewResult(app) r.Error = fmt.Sprintf("failed while determining the application should be triggered or not, %v", err) failedResults = append(failedResults, r) } diff --git a/pkg/app/piped/platformprovider/kubernetes/state.go b/pkg/app/piped/platformprovider/kubernetes/state.go index 22534073e3..c1711b9bd3 100644 --- a/pkg/app/piped/platformprovider/kubernetes/state.go +++ b/pkg/app/piped/platformprovider/kubernetes/state.go @@ -30,7 +30,7 @@ import ( "github.com/pipe-cd/pipecd/pkg/model" ) -func MakeKubernetesResourceState(uid string, key ResourceKey, obj *unstructured.Unstructured, now time.Time) model.KubernetesResourceState { +func MakeKubernetesResourceState(uid string, key ResourceKey, obj *unstructured.Unstructured, now time.Time) *model.KubernetesResourceState { var ( owners = obj.GetOwnerReferences() ownerIDs = make([]string, 0, len(owners)) @@ -43,7 +43,7 @@ func MakeKubernetesResourceState(uid string, key ResourceKey, obj *unstructured. } sort.Strings(ownerIDs) - state := model.KubernetesResourceState{ + state := &model.KubernetesResourceState{ Id: uid, OwnerIds: ownerIDs, // TODO: Think about adding more parents by using label selectors diff --git a/pkg/app/pipedv1/planpreview/builder.go b/pkg/app/pipedv1/planpreview/builder.go index 46f8282d43..97fd2e9f72 100644 --- a/pkg/app/pipedv1/planpreview/builder.go +++ b/pkg/app/pipedv1/planpreview/builder.go @@ -219,7 +219,7 @@ func (b *builder) buildApp(ctx context.Context, worker int, command string, app logger.Info("will decide sync strategy for an application") - result = model.MakeApplicationPlanPreviewResult(*app) + result = model.MakeApplicationPlanPreviewResult(app) var preCommit string // Find the commit of the last successful deployment. @@ -361,7 +361,7 @@ func (b *builder) findTriggerApps(ctx context.Context, repo git.Repo, apps []*mo continue } - r := model.MakeApplicationPlanPreviewResult(*app) + r := model.MakeApplicationPlanPreviewResult(app) r.Error = fmt.Sprintf("failed while determining the application should be triggered or not, %v", err) failedResults = append(failedResults, r) } diff --git a/pkg/app/server/applicationlivestatestore/store.go b/pkg/app/server/applicationlivestatestore/store.go index c5b067ec06..08255de63b 100644 --- a/pkg/app/server/applicationlivestatestore/store.go +++ b/pkg/app/server/applicationlivestatestore/store.go @@ -108,7 +108,7 @@ func (s *store) PatchKubernetesApplicationLiveState(ctx context.Context, events snapshot = ss snapshots[ev.ApplicationId] = ss } - if ev.SnapshotVersion.IsBefore(*snapshot.Version) { + if ev.SnapshotVersion.IsBefore(snapshot.Version) { continue } switch ev.Type { diff --git a/pkg/model/application.go b/pkg/model/application.go index 5c9651396e..46fe515631 100644 --- a/pkg/model/application.go +++ b/pkg/model/application.go @@ -28,11 +28,11 @@ const ( ) // GetApplicationConfigFilePath returns the path to application configuration file. -func (p ApplicationGitPath) GetApplicationConfigFilePath() string { +func (p *ApplicationGitPath) GetApplicationConfigFilePath() string { return filepath.Join(p.Path, p.GetApplicationConfigFilename()) } -func (p ApplicationGitPath) GetApplicationConfigFilename() string { +func (p *ApplicationGitPath) GetApplicationConfigFilename() string { // The config file name used to allow to be empty until the default name got changed. // So empty means the old default name. filename := oldDefaultApplicationConfigFilename @@ -44,7 +44,7 @@ func (p ApplicationGitPath) GetApplicationConfigFilename() string { // HasChanged checks whether the content of sync state has been changed. // This ignores the timestamp value. -func (s ApplicationSyncState) HasChanged(next ApplicationSyncState) bool { +func (s *ApplicationSyncState) HasChanged(next *ApplicationSyncState) bool { if s.Status != next.Status { return true } diff --git a/pkg/model/application_live_state.go b/pkg/model/application_live_state.go index e0b31c8b66..0dbe6b582f 100644 --- a/pkg/model/application_live_state.go +++ b/pkg/model/application_live_state.go @@ -14,7 +14,7 @@ package model -func (v ApplicationLiveStateVersion) IsBefore(a ApplicationLiveStateVersion) bool { +func (v *ApplicationLiveStateVersion) IsBefore(a *ApplicationLiveStateVersion) bool { if v.Timestamp < a.Timestamp { return true } @@ -24,7 +24,7 @@ func (v ApplicationLiveStateVersion) IsBefore(a ApplicationLiveStateVersion) boo return v.Index < a.Index } -func (s KubernetesResourceState) HasDiff(a KubernetesResourceState) bool { +func (s *KubernetesResourceState) HasDiff(a *KubernetesResourceState) bool { if s.ApiVersion != a.ApiVersion { return true } diff --git a/pkg/model/application_live_state_test.go b/pkg/model/application_live_state_test.go index 4da42c9545..421c9c1a39 100644 --- a/pkg/model/application_live_state_test.go +++ b/pkg/model/application_live_state_test.go @@ -207,9 +207,10 @@ func TestKubernetesResourceState_HasDiff(t *testing.T) { want: true, }, } - for _, tc := range testcases { + for i := range testcases { + tc := &testcases[i] t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, tc.s.HasDiff(tc.a)) + assert.Equal(t, tc.want, tc.s.HasDiff(&tc.a)) }) } } diff --git a/pkg/model/application_test.go b/pkg/model/application_test.go index 82027b09b0..4ca46c1a90 100644 --- a/pkg/model/application_test.go +++ b/pkg/model/application_test.go @@ -229,11 +229,12 @@ func TestHasChanged(t *testing.T) { }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { t.Parallel() - actual := tt.current.HasChanged(tt.next) + actual := tt.current.HasChanged(&tt.next) assert.Equal(t, tt.expected, actual) }) } @@ -259,7 +260,8 @@ func TestGetApplicationConfigFilename(t *testing.T) { }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { t.Parallel() diff --git a/pkg/model/deployment_chain_test.go b/pkg/model/deployment_chain_test.go index f633b524f5..2202fbca29 100644 --- a/pkg/model/deployment_chain_test.go +++ b/pkg/model/deployment_chain_test.go @@ -104,8 +104,8 @@ func TestDeploymentChainDesireStatus(t *testing.T) { }, } - for _, tc := range testcases { - tc := tc + for i := range testcases { + tc := &testcases[i] t.Run(tc.name, func(t *testing.T) { t.Parallel() desireStatus := tc.deploymentChain.DesiredStatus() diff --git a/pkg/model/deployment_test.go b/pkg/model/deployment_test.go index e39f0d5d22..0e97be810b 100644 --- a/pkg/model/deployment_test.go +++ b/pkg/model/deployment_test.go @@ -453,7 +453,8 @@ func TestTriggeredBy(t *testing.T) { }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { d := &Deployment{ Trigger: &tt.trigger, @@ -554,7 +555,8 @@ func TestTriggerBefore(t *testing.T) { }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { got := tt.d.TriggerBefore(&tt.other) assert.Equal(t, tt.want, got) diff --git a/pkg/model/piped_test.go b/pkg/model/piped_test.go index 78dc0ceff1..ad263e7220 100644 --- a/pkg/model/piped_test.go +++ b/pkg/model/piped_test.go @@ -164,10 +164,11 @@ func TestPipedDeleteOldPipedKeys(t *testing.T) { }, } - for _, tc := range testcases { + for i := range testcases { + tc := &testcases[i] t.Run(tc.name, func(t *testing.T) { tc.piped.DeleteOldPipedKeys() - assert.Equal(t, tc.expected, tc.piped) + assert.Equal(t, &tc.expected, &tc.piped) }) } } @@ -221,10 +222,11 @@ func TestPipedRedactSensitiveData(t *testing.T) { }, } - for _, tc := range testcases { + for i := range testcases { + tc := &testcases[i] t.Run(tc.name, func(t *testing.T) { tc.piped.RedactSensitiveData() - assert.Equal(t, tc.expected, tc.piped) + assert.Equal(t, &tc.expected, &tc.piped) }) } } diff --git a/pkg/model/planpreview.go b/pkg/model/planpreview.go index 289e1ce3f2..9b8a96cca3 100644 --- a/pkg/model/planpreview.go +++ b/pkg/model/planpreview.go @@ -23,7 +23,7 @@ func (r *PlanPreviewCommandResult) FillURLs(baseURL string) { } } -func MakeApplicationPlanPreviewResult(app Application) *ApplicationPlanPreviewResult { +func MakeApplicationPlanPreviewResult(app *Application) *ApplicationPlanPreviewResult { r := &ApplicationPlanPreviewResult{ ApplicationId: app.Id, ApplicationName: app.Name, diff --git a/pkg/model/project_test.go b/pkg/model/project_test.go index 5948c7a096..37fb47809e 100644 --- a/pkg/model/project_test.go +++ b/pkg/model/project_test.go @@ -904,7 +904,8 @@ func TestGenerateAuthCodeURL_Oidc(t *testing.T) { }, } - for _, tt := range tests { + for i := range tests { + tt := &tests[i] t.Run(tt.name, func(t *testing.T) { authURL, err := tt.config.GenerateAuthCodeURL(tt.project, tt.state) if tt.expectedError { From 60d23b7f466d9e2e82d1910aa1a4d89774a4f5cf Mon Sep 17 00:00:00 2001 From: srinivasr Date: Tue, 25 Aug 2026 23:03:58 +0530 Subject: [PATCH 2/2] add note about returning pointers to internal state Signed-off-by: srinivasr --- pkg/app/piped/livestatestore/kubernetes/store.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/app/piped/livestatestore/kubernetes/store.go b/pkg/app/piped/livestatestore/kubernetes/store.go index 1ef5ca7cf0..5d1ebab16c 100644 --- a/pkg/app/piped/livestatestore/kubernetes/store.go +++ b/pkg/app/piped/livestatestore/kubernetes/store.go @@ -290,6 +290,7 @@ func (s *store) getAppLiveState(appID string) (AppState, bool) { resources = make([]*model.KubernetesResourceState, 0, len(nodes)) ) for i := range nodes { + // Note: The returned states are pointers shared with the store's internal data, treat them as read-only resources = append(resources, nodes[i].state) }