-
Notifications
You must be signed in to change notification settings - Fork 78
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
(maint) Handle an invalid mirror gracefully #813
Conversation
fa96d7d
to
2b47423
Compare
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.
LGTM, but please add a CHANGELOG entry.
lib/vanagon/component.rb
Outdated
@@ -260,14 +260,28 @@ def fetch_mirrors(options) | |||
VanagonLogger.info %(Attempting to fetch from mirror URL "#{mirror}") | |||
@source = Vanagon::Component::Source.source(mirror, **options) | |||
return true if source.fetch | |||
rescue Vanagon::Error => e | |||
if e.message =~ /Unknown file type/ |
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.
I think I'd prefer raising an subclass of Vanagon::Error
specifically for the unknown source case and rescue the most specific exceptions first like:
rescue Vanagon::UnknownSourceError
VanagonLogger.error %(Unable to access mirror URL "#{mirror}")
rescue Vanagon::Error
raise
rescue SocketError
...
rescue StandardError
...
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.
Yeah, that's fair. I was being lazy :)
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.
Done
2b47423
to
b25ebff
Compare
b25ebff
to
60037c5
Compare
60037c5
to
8d4a9ee
Compare
Because the HTTP source's valid_url? check was always returning truthy before, a mirror URL to a file would always seem to be valid, even if it wasn't. Now that puppetlabs@9995d5e fixed it, a Vanagon::Error would get thrown saying it was an unknown file type. This means that fetch_url would never get called since this exception in fetch_mirrors would be unhandled. This handles that particular error, and continues to throw other Vanagon::Error types, as they usually indicate a problem with the definition of the component or some other type of fatal error. Additionally, this changes the catching of RuntimeError to StandardError, so that we can handle issues talking to the particular URL, such as OpenSSL errors.
8d4a9ee
to
dbd2721
Compare
Because the HTTP source's valid_url? check was always returning truthy before, a mirror URL to a file would always seem to be valid, even if it wasn't. Now that 9995d5e fixed it, a Vanagon::Error would get thrown saying it was an unknown file type. This means that fetch_url would never get called since this exception in fetch_mirrors would be unhandled.
This handles that particular error, and continues to throw other Vanagon::Error types, as they usually indicate a problem with the definition of the component or some other type of fatal error. Additionally, this changes the catching of RuntimeError to StandardError, so that we can handle issues talking to the particular URL, such as OpenSSL errors.
Please add all notable changes to the "Unreleased" section of the CHANGELOG.