bug: pipedv1 deployment store returns newest deployment as head, not oldest

オープン
  • 何か問題がありますか？
#6,780 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 2 日以内に返信

まだ誰も着手していません。

  • #6921 @AruneshDwivedi による — マージされずにクローズ

評価

難易度
4/5
見積もり時間
3〜5日
初心者へのやさしさ
48/100
issue の種類
バグ
明瞭さ
おおむね明確
活発さ
静か
技術スタック
go
領域
devops, infrastructure

調査の方向性

pkg/app/pipedv1/apistore/deploymentstore/store.go:125-134 の ListAppHeadDeployments と、pkg/app/piped/apistore/deploymentstore/store.go の対応する実装から始めます。メソッドがどこで使用されているかを追跡し、head が最も古い未完了の deployment と最も新しい未完了の deployment のどちらであるべきかを判断してから、issue に記載されている 2 つの deployment を使った回帰テストを両方の store に追加します。実装と Lister の docstring が一致していれば完了です。

索引モデルが issue の本文から書いたものです。

説明

What happened:

While reviewing the deployment store sync logic in pipedv1, the head-deployment selection in pkg/app/pipedv1/apistore/deploymentstore/store.go:125-134 looks inverted relative to the Lister interface's docstring.

The interface promises:

ListAppHeadDeployments returns the map from application ID to its head of deploying deployments.
Head deployment is same with the oldest uncompleted one.

But the implementation writes pendings → planneds → runnings into the same map[string]*model.Deployment, so for an application with both a pending and a running deployment, the running one wins by overwrite order:

for _, d := range pendings { headDeployments[d.ApplicationId] = d }
for _, d := range planneds { headDeployments[d.ApplicationId] = d }
for _, d := range runnings { headDeployments[d.ApplicationId] = d }

A PENDING deployment is logically newer than a RUNNING one for the same application (it was created later, after the running one started), so this code returns the newest uncompleted deployment, not the oldest.

The same inversion exists at the corresponding location in the v0 store (pkg/app/piped/apistore/deploymentstore/store.go).

What you expected to happen:

Either:

  1. The docstring is updated to match the implementation ("head = newest uncompleted"), or
  2. The implementation is corrected to match the docstring — either reverse the write order or pick the head explicitly by created_at.

I don't know which side is correct, since I haven't traced where ListAppHeadDeployments is consumed downstream. Whichever way it ends up, the docstring and the code should agree.

How to reproduce it:

Unit test against the store with two *model.Deployment instances sharing one ApplicationId, one DEPLOYMENT_PENDING and one DEPLOYMENT_RUNNING. Lister.ListAppHeadDeployments()[appID] returns the running one — the opposite of what the docstring promises.

Environment:

  • piped version: master @ 78fdbeb7c (both v0 and v1 affected)
  • control-plane version: same
  • Others: surfaced during self-review of #6727 while writing tests for ListAppHeadDeployments
主要言語
Go
スター
1.4k
フォーク
391
平均マージ
4日 1時間
マージ済み PR（30日）
58

環境構築

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

pipe-cd/pipecd のほかの issue

pipe-cd/pipecd の issue をすべて見る

似ている issue

Go の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。