Create Audit Log FSM state #344
Conversation
kyma-bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
kyma-bot
added
cla: yes
kyma-bot
added
size/XL
| m.log.Info("Configure Audit Log state") | ||
|
|
||
| wasAuditLogEnabled, err := m.AuditLogging.Enable(ctx, s.shoot) | ||
| if err != nil && !wasAuditLogEnabled { |
There was a problem hiding this comment.
I think we can replace if err != nil && !wasAuditLogEnabled with if err != nil .
kyma-bot
added
size/XXL
| @@ -94,6 +94,9 @@ const ( | |||
| ConditionReasonKubernetesAPIErr = RuntimeConditionReason("KubernetesErr") | |||
| ConditionReasonSerializationError = RuntimeConditionReason("SerializationErr") | |||
| ConditionReasonDeleted = RuntimeConditionReason("Deleted") | |||
|
|
|||
| ConditionReasonAdministratorsConfigured = RuntimeConditionReason("AdministratorsConfigured") | |||
There was a problem hiding this comment.
I can't find the usage of it - was this addition intentional?
| return | ||
| } | ||
|
|
||
| if cfg.Kind == extensionKind && |
There was a problem hiding this comment.
How about extracting this check to some separate method like
isExtensionModified to make this method shorter and possibly make the unit testing easier?
| if wasAuditLogEnabled { | ||
| m.log.Info("Audit Log configured for shoot: " + s.shoot.Name) | ||
| s.instance.UpdateStateReady( | ||
| imv1.ConditionTypeRuntimeConfigured, |
There was a problem hiding this comment.
I'm thinking if adding an additional ConditionType similar to ConditionTypeRuntimeKubeconfigReady wouldn't be beneficial. If there will be an issue with the cluster we could then quickly see if this might be connected to audit logs.
| } | ||
|
|
||
| func (t *CustomTracker) Delete(gvr schema.GroupVersionResource, ns, name string) error { | ||
| t.mu.Lock() | ||
| defer t.mu.Unlock() | ||
|
|
||
| if gvr.Resource == "shoots" { | ||
| if gvr.Resource == "shoots" { //nolint:goconst |
There was a problem hiding this comment.
Is there any downside of using a const here?
| t.mu.Lock() | ||
| defer t.mu.Unlock() | ||
|
|
||
| if gvr.Resource == "shoots" { //nolint:goconst |
There was a problem hiding this comment.
Is there any downside of using a const here?
| if t.callCnt < len(t.shootSequence) { | ||
| shoot := t.shootSequence[t.callCnt] | ||
| t.callCnt++ | ||
| if gvr.Resource == "shoots" { //nolint:goconst |
There was a problem hiding this comment.
Is there any downside of using a const here?
There was a problem hiding this comment.
I will change this to const
| func (t *CustomTracker) IsSequenceFullyUsed() bool { | ||
| return t.callCnt == len(t.shootSequence) && len(t.shootSequence) > 0 | ||
| return (t.shootCallCnt == len(t.shootSequence) && len(t.shootSequence) > 0) && t.seedCallCnt == len(t.seedSequence) | ||
| } | ||
|
|
||
| func (t *CustomTracker) Get(gvr schema.GroupVersionResource, ns, name string) (runtime.Object, error) { |
There was a problem hiding this comment.
Have you considered writing unit tests for this Get and getNextObject methods?
There was a problem hiding this comment.
Thanks for hint, I will add those unit test
kyma-bot
removed
the
do-not-merge/work-in-progress
|
/cla-recheck |
Description
Changes proposed in this pull request:
Related issue(s)
#286