cover sync/rollback scenarios #1
Conversation
Signed-off-by: Zadkiel AHARONIAN <hello@zadkiel.fr> Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Zadkiel AHARONIAN <hello@zadkiel.fr> Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Zadkiel AHARONIAN <hello@zadkiel.fr> Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Zadkiel AHARONIAN <hello@zadkiel.fr> Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
… with new commit Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
… with new commit Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
…494-agaudreault Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| command.Flags().DurationVar(&opts.retryBackoffDuration, "sync-retry-backoff-duration", argoappv1.DefaultSyncRetryDuration, "Sync retry backoff base duration. Input needs to be a duration (e.g. 2m, 1h)") | ||
| command.Flags().DurationVar(&opts.retryBackoffMaxDuration, "sync-retry-backoff-max-duration", argoappv1.DefaultSyncRetryMaxDuration, "Max sync retry backoff duration. Input needs to be a duration (e.g. 2m, 1h)") | ||
| command.Flags().Int64Var(&opts.retryBackoffFactor, "sync-retry-backoff-factor", argoappv1.DefaultSyncRetryFactor, "Factor multiplies the base duration after each failed sync retry") | ||
| command.Flags().BoolVar(&opts.retryRefresh, "sync-retry-refresh", false, "Set if a new revision should trigger a new sync") | ||
| command.Flags().BoolVar(&opts.retryRefresh, "sync-retry-refresh", false, "Indicates if the latest revision should be used on retry instead of the initial one") | ||
| command.Flags().StringVar(&opts.ref, "ref", "", "Ref is reference to another source within sources field") | ||
| command.Flags().StringVar(&opts.SourceName, "source-name", "", "Name of the source from the list of sources of the app.") | ||
| } |
There was a problem hiding this comment.
The implementation of retry logic lacks validation for the duration and factor values. It's important to ensure that these values are within a reasonable range to prevent potential issues such as excessively long retry intervals or overly aggressive retry escalation, which could impact application performance and resource usage.
Recommendation: Implement validation checks for retryBackoffDuration, retryBackoffMaxDuration, and retryBackoffFactor to ensure they fall within expected limits. This could be done by adding a check after the flags are parsed or by using a custom flag parsing function that includes validation logic.
| . "github.com/argoproj/gitops-engine/pkg/sync/common" | ||
| "github.com/stretchr/testify/assert" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/utils/ptr" | ||
|
|
||
| . "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" |
There was a problem hiding this comment.
The use of dot imports (import . "package") can lead to namespace pollution, making it difficult to track where identifiers are defined. Consider importing packages without the dot to enhance code clarity and maintainability.
Suggested Change:
Replace dot imports with standard imports and use package names to access exported identifiers. This change will make the codebase easier to understand and maintain.
| Retry: &RetryStrategy{ | ||
| Limit: -1, | ||
| Refresh: true, | ||
| Backoff: &Backoff{ | ||
| Duration: time.Second.String(), | ||
| Factor: ptr.To(int64(1)), | ||
| MaxDuration: time.Second.String(), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Setting the retry limit to -1 in RetryStrategy suggests an infinite retry loop. This configuration can lead to undesirable behavior, especially in production-like environments where a failing sync could potentially loop indefinitely without resolution.
Suggested Change:
Consider implementing a sensible upper limit for retries or include conditions under which retries should be aborted. This approach will prevent potential infinite loops and ensure that the system can recover or alert for intervention when necessary.
| } | ||
| } | ||
|
|
||
| func Status(f func(v1alpha1.ApplicationStatus) (bool, string)) Expectation { | ||
| return func(c *Consequences) (state, string) { | ||
| ok, msg := f(c.app().Status) | ||
| if !ok { | ||
| return pending, msg | ||
| } | ||
| return succeeded, msg | ||
| } | ||
| } | ||
|
|
||
| func Namespace(name string, block func(app *v1alpha1.Application, ns *corev1.Namespace)) Expectation { | ||
| return func(c *Consequences) (state, string) { | ||
| ns, err := namespace(name) |
There was a problem hiding this comment.
Error Handling Improvement
The error handling in the function that retrieves and operates on a namespace could be improved. Currently, if the namespace function returns an error, it is directly appended to a failure message. This approach might not provide enough context or detail about the error, especially in a complex system with multiple potential points of failure.
Recommendation:
Consider categorizing errors (e.g., network issues, permissions, non-existent resources) and providing more specific error messages or recovery suggestions. This would make debugging easier and improve the robustness of the error handling.
| } | ||
| return func(c *Consequences) (state, string) { | ||
| if c.actions.lastError != nil { | ||
| return failed, "error" | ||
| return failed, fmt.Errorf("error: %w", c.actions.lastError).Error() | ||
| } | ||
| if !match(c.actions.lastOutput, message) { | ||
| return failed, fmt.Sprintf("output did not contain '%s'", message) |
There was a problem hiding this comment.
Logic and Maintainability Enhancement
The Success function checks if the last command was successful based on the absence of an error and whether the output contains a specified message. However, if the output does not contain the expected message, the error message simply states that the output did not contain the specified string, without providing further context or details about what was actually in the output.
Recommendation:
Enhance the error message to include what was actually in the output when the expected message is not found. This would provide immediate feedback on what went wrong, making the debugging process more straightforward and the code more maintainable.
…494-agaudreault Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
3 participants
Checklist: