-
Notifications
You must be signed in to change notification settings - Fork 70
Feat: add .status.observedGeneration to Configuration #269
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
Codecov Report
@@ Coverage Diff @@
## master #269 +/- ##
==========================================
- Coverage 77.44% 75.14% -2.31%
==========================================
Files 19 19
Lines 1215 1219 +4
==========================================
- Hits 941 916 -25
- Misses 202 240 +38
+ Partials 72 63 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
This feature is pretty cool, which I planned to implement tonight:) |
|
@lowkeyrd Please help review this feature. |
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| var tfExecutionJob = &batchv1.Job{} |
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.
这一块不能删除,它解决的是这个场景:
Configuration 部署过程中,job 已经完成了,状态还是 ProvisioningAndChecking,这时候删除 Configuration,状态就无法到达 Avaialable,导致状态 block,无法部署成功也无法删除。
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.
这里需要判断一下是否是当前 spec 对应的 job ,如果新的 spec 进来会后会拿到上一次的 job
| for k, v := range variableInSecret.Data { | ||
| if val, ok := meta.VariableSecretData[k]; !ok || !bytes.Equal(v, val) { | ||
| envChanged = true | ||
| meta.EnvChanged = true |
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.
这个一旦设置为 true,就无法变成 false,所以不能设置为全局的属性。
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.
每次事件触发后进来都会初始化 meta。
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.
嗯嗯,我的意思就是一次 reconcile,你在这一行加了一个判断是更合理的。
https://github.com/oam-dev/terraform-controller/pull/269/files#diff-1aa78f86e236bbfbdd87a2dd01cfad0cda0964bdfc8ba4a7ba036fd11cbbabb5R267
21a8da5 to
7269a1d
Compare
| } | ||
|
|
||
| // Check whether env changes | ||
| if err := meta.prepareTFVariables(configuration); err != 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.
Great, I did move it to here in another pending PR.
| break | ||
| } | ||
| } | ||
| } else if !kerrors.IsNotFound(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.
What will happen if it's not IsNotFound error?
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.
system err or runtime 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.
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.
如果 not found 在这里返回,会无法继续进行;后面会在配置变更后删除 secrets object
| return err | ||
| } | ||
|
|
||
| break |
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.
Good catch!
Signed-off-by: Evan Li <evan.li97@outlook.com>
zzxwill
left a comment
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.
Great job!
- Change json tag of spec.Region to `customRegion` in Configuration kubevela#3384 - Use .status.observedGeneration to make Terraform outputs is latest kubevela/terraform-controller#269 Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
- Change json tag of spec.Region to `customRegion` in Configuration kubevela#3384 - Use .status.observedGeneration to make Terraform outputs is latest kubevela/terraform-controller#269 Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
* Upgrade API version for Terraform Controller - Change json tag of spec.Region to `customRegion` in Configuration #3384 - Use .status.observedGeneration to make Terraform outputs is latest kubevela/terraform-controller#269 Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com> * fix ci Signed-off-by: Zheng Xi Zhou <zzxwill@gmail.com>
Add .status.observedGeneration to Configuration.
reference:
Signed-off-by: Evan Li evan.li97@outlook.com