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

Support fallback URLs for netkan validate-ckan #3060

Merged
merged 3 commits into from
May 24, 2020

Conversation

HebaruSan
Copy link
Member

Problem

If you submit a CKAN-meta pull request for a module that currently relies on its archive.org fallback URL for download, the pull request validation will fail, even though the module can be installed by the client.

Cause

Netkan uses a downloading service separate from the client's, which does not know about fallback URLs.

Changes

  • Now Metadata provides a FallbackDownload property that returns an archive.org Uri if we have enough info to do so and null otherwise.
  • Now CachingHttpService takes Metadata as input when downloading instead of a Uri, identifier, and timestamp separately.
  • Now if a download fails, CachingHttpService attempts to get FallbackDownload if it is set, and propagates the exception otherwise.

This will allow netkan.exe --validate-ckan to access fallback URLs and pass validation for modules relying on them.

NOTE: I just realized I forgot to check for redistributable licenses. I'll start that and remove the "In Progress" label when done.
NOTE: Also, the cache entries should be stored according to the original URL, not the fallback. Consider this pending as well.

Fixes #2938.

@HebaruSan HebaruSan added Enhancement Pull request In progress We're still working on this Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN labels May 23, 2020
@HebaruSan HebaruSan requested a review from DasSkelett May 23, 2020 01:02
@HebaruSan HebaruSan removed the In progress We're still working on this label May 23, 2020
}
else
{
return DownloadPackage(fallback, metadata.Identifier, metadata.RemoteTimestamp, metadata.Download);
Copy link
Member

Choose a reason for hiding this comment

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

A log statement would probably be handy here, so that you can see that the fallback is used.
I would go for a Log.Info at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, borrowed the one from Core.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Neat, should speed up CKAN-meta work

@HebaruSan HebaruSan merged commit 6273ca5 into KSP-CKAN:master May 24, 2020
@HebaruSan HebaruSan deleted the fix/netkan-fallback branch May 24, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN Pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants