-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix: add canary_id to job_history and cleanup sync canary function #1169
Conversation
canary, err := c.ToV1() | ||
if err != nil { | ||
logger.Errorf("Error parsing canary[%s]: %v", c.ID, err) | ||
jobHistory.AddError(err.Error()) | ||
continue | ||
} |
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 required, we are doing this in the SyncCanary func
if len(canary.Status.Checks) == 0 && len(canary.Spec.GetAllChecks()) > 0 { | ||
logger.Infof("Persisting %s as it has no checks", canary.Name) | ||
pkgCanary, _, _, err := db.PersistCanary(*canary, canary.Annotations["source"]) | ||
if err != nil { | ||
logger.Errorf("Failed to persist canary %s: %v", canary.Name, err) | ||
jobHistory.AddError(err.Error()) | ||
continue | ||
} |
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.
Since these canaries are always fetched from database, no persistence logic is needed
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.
@yashmehrotra we were also saving checks from the canary spec in here. Now, checks aren't being saved.
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.
@yashmehrotra I'm not talking about component checks. If you create a canary from the UI then the checks are never saved.
v1canary, err := pkgCanary.ToV1() | ||
if err != nil { | ||
logger.Errorf("Failed to convert canary to V1 %s: %v", canary.Name, err) | ||
jobHistory.AddError(err.Error()) | ||
continue | ||
} |
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.
redundant conversion
Fixes: flanksource/duty#229
@moshloop @adityathebe
Removed some dead logic and redundant conditions which I don't think was necessary. Please do a double check on this