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

Added retry to update operation #193

Merged
merged 2 commits into from
Mar 6, 2014
Merged

Added retry to update operation #193

merged 2 commits into from
Mar 6, 2014

Conversation

ianunruh
Copy link

@ianunruh ianunruh commented Dec 5, 2013

Sometimes I'll get transient DNS or HTTP errors when apt-get update runs. This is an attempt to compensate for these errors.

@daenney
Copy link

daenney commented Feb 25, 2014

I'm not a big fan of solving transient DNS or HTTP errors though 'oh heck just retry it' instead of finding the root cause of the issue but it can't really hurt either in this case.

@apenney @hunner: What do you guys think?

Btw the tests are failing on a syntax error in force_spec.rb from way back.

@daenney
Copy link

daenney commented Mar 5, 2014

We discussed this is little and as-is we can't merge it. For most people a retry count of 1 is what they expect. In my case I wouldn't want a transient DNS error or whatever to cause another pass.

So, this should become configurable. It should be a parameter to the apt class which should default to either 1 or undef, in both cases that would cause the retry to be set to 1 since exec resources default to 1 if nothing is passed in.

@ianunruh
Copy link
Author

ianunruh commented Mar 5, 2014

I do agree that 3 tries was a bit excessive. In that case, I would just make the default be undef and let users decide how many times they want to retry.

@hunner
Copy link

hunner commented Mar 5, 2014

Thanks for updating this! Looks like it will need a rebase to merge though. Also, could you make sure the tests pass? @daenney mentions the force_spec.rb file will need updating; you can run them yourself by running bundle install && bundle exec rake spec in the module directory.

@daenney
Copy link

daenney commented Mar 5, 2014

Actually, what I meant was that the original reason for the tests to fail on his pull request was a syntax error in force_spec.rb at the time. That syntax error was not @ianunruh's doing though.

In order to rebase your commit:

git remote add upstream https://github.com/puppetlabs/puppetlabs-apt.git
git checkout master
git fetch upstream
git merge upstream/master
git checkout feature/retry-update
git rebase master

@daenney
Copy link

daenney commented Mar 6, 2014

As far as I'm concerned, this is good to go.

hunner added a commit that referenced this pull request Mar 6, 2014
@hunner hunner merged commit 4d2819f into puppetlabs:master Mar 6, 2014
@LukasAud LukasAud added the bugfix label Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants