Skip to content
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

Add retries to circleci CRUD resources #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ajlake
Copy link

@ajlake ajlake commented May 5, 2020

CircleCI occasionally gives back transient http 500 errors, resulting in terraform plans/applies failing. This only gets worse as the number of CircleCI variables in a terraform workspace grows. This change takes advantage of terraform's native retry functionality to wrap CRUD operations in an attempt to mitigate the issue.

Copy link
Contributor

@tgermain tgermain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, my only question is for the env var creation retry.

Comment on lines +185 to +196
exists, err := providerClient.EnvVarExists(organization, projectName, envName)
if err != nil {
return wrap(err)
}

if exists {
return fmt.Errorf("environment variable '%s' already exists for project '%s'", envName, projectName)
}
if exists {
return wrap(fmt.Errorf("environment variable '%s' already exists for project '%s'", envName, projectName))
}

if _, err := providerClient.AddEnvVar(organization, projectName, envName, envValue); err != nil {
return err
}
if _, err := providerClient.AddEnvVar(organization, projectName, envName, envValue); err != nil {
return wrap(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we split the retry here and use a different, one for the call to EnvVarExists() and another retry for AddEnvVar() ?

If the first one succeed (and return false) but the second one fail due to network issue (aka a retryable error), should we do the first request again too?
I'm not sure if it's overkill to do 2 different retries and what we gain from doing only one.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splitting makes sense to me

@mrolla
Copy link
Owner

mrolla commented May 17, 2020

@ajlake would you mind rebasing on master? If you are going to split the two retries please also do that so we can merge this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants