IACS-295 Revert "Elide last applied configuration annotation when SSA is supported" #1
Conversation
kaz130
changed the title
| if k.serverSideApplyMode || k.supportsDryRun(config.GroupVersionKind()) { | ||
| // Skip last-applied-config annotation if the resource supports server-side apply. | ||
| return config, nil | ||
| } | ||
|
|
||
| // CRDs are updated using a separate mechanism, so skip the last-applied-configuration annotation, and delete it | ||
| // if it was present from a previous update. | ||
| if clients.IsCRD(config) { | ||
| // Deep copy the config before returning. | ||
| config = config.DeepCopy() | ||
|
|
||
| annotations := getAnnotations(config) | ||
| delete(annotations, lastAppliedConfigKey) | ||
| config.SetAnnotations(annotations) | ||
| return config, nil | ||
| } |
There was a problem hiding this comment.
revertした影響でメソッドから関数に変わっておりちょっと分かりづらいですが、下のwithLastAppliedConfigと比較するとannotaationの設定をスキップする部分を削除しています。
There was a problem hiding this comment.
Revertしただけでは解決せず、以下の処理を加える必要があったということでしょうか？
annotaationの設定をスキップする部分を削除し
There was a problem hiding this comment.
Revertすることで「annotationの設定をスキップする部分が削除される」という意図でした。
|
d884482 について
|
|
d4b1caf について |
pulumi#1863 に含まれる変更の内、last-applied-configurationを書き出さないようにする変更( pulumi@e9115d9 )はrevertしたままにして、変数名と一部ロジックの修正( pulumi@87b009f )だけ中途半端にrevert前に戻そうとしてました。
を新たに積む形で「ロジックに変更のないPRとして最終的なDiffが減るようにした部分」は削除しようと思います。 |
| @@ -2785,39 +2788,6 @@ func (k *kubeProvider) tryServerSidePatch( | |||
| return nil, nil, false, err | |||
| } | |||
|
|
|||
| func (k *kubeProvider) withLastAppliedConfig(config *unstructured.Unstructured) (*unstructured.Unstructured, error) { | |||
There was a problem hiding this comment.
ここ、私がrevertするときにconflictをうまく解消できていなかったせいでややこしくなっているポイントです。
元の履歴だと以下のようになっています。
（現在）
- Don't use the last-applied-configuration annotation for CRDs pulumi/pulumi-kubernetes#1882
- withLastAppliedConfigメソッドでlast-applied-configの書き出しのスキップをする際のCRDに関する条件分岐を追加
- Elide last applied configuration annotation when SSA is supported pulumi/pulumi-kubernetes#1863
- last-applied-config を書き出さないように変更
- その際にwithLastAppliedConfig関数を削除し、withLastAppliedConfigメソッドに変更
（過去）
そのため、単純に pulumi#1863 をrevertするとconflictが発生したため、一旦は両方のwithLastAppliedConfigを残すようにconflictを解消しました。
その後、withLastAppliedConfigメソッドの方はどこからも呼び出されないため不要と判断し、CRDに関する条件分岐もろともwithLastAppliedConfigメソッドを削除したのが上記のコミットになります。
|
本PRと直接関係ないですが、tagがpushされていると、どこからブランチが切ってるかが分かりやすくて良いかと思います。 |
tests/sdk/nodejs/nodejs_test.go
Outdated
| func TestDryRun(t *testing.T) { | ||
| test := baseOptions.With(integration.ProgramTestOptions{ | ||
| Dir: filepath.Join("dry-run", "step1"), | ||
| EditDirs: []integration.EditDir{ | ||
| { | ||
| Dir: filepath.Join("dry-run", "step2"), | ||
| Additive: true, | ||
| }, | ||
| }, | ||
| }) | ||
| integration.ProgramTest(t, &test) | ||
| } | ||
|
|
There was a problem hiding this comment.
あまりロジック部分には関係ないですが、このテストは、pulumi@11e356e のコミットの内容には存在しないようです。
確かにRevertコマンド打つと本テストが出てきますが、これって入れるべきなんですかね...?
どういう原理でRevertで出てきているんでしょうか...
There was a problem hiding this comment.
これは pulumi#1863 でテストの中に処理を追加し、その後の別の修正でテストが削除されたというコミットが積まれているみたいです。
なので pulumi#1863 だけrevertするとテストの中に追加された処理だけが中途半端に復活してしまうため、テスト全てを復活させるか、全て無かったことにするかしてconflictを解消する必要があった感じですね。今回は全て復活させましたが、sync forkする時に邪魔になりそうなので削除しておきます。
…rted (pulumi#1863)" This reverts commit 11e356e.
kaz130
force-pushed
the
IACS-295-enable-last-applied
branch
from
deab87a
to
2091815
Compare
概要
https://m-pipe.atlassian.net/browse/IACS-295
last-applied-configuration を書き出さないようにするPR (pulumi#1863) をrevertしました。
deployment-workerの修正と合わせて更新してリソースが正しく更新できることを確認しています。
また、test (make test_provider) が通ることも確認しています。