-
Notifications
You must be signed in to change notification settings - Fork 47
Remove Metadata requirement for IAs to run in DuckPAN #383
Conversation
@zachthompson @mintsoft I've pushed some commits. This now prevents the message and DuckPAN Query works fine. However, for DuckPAN Server, we have a few more direct calls to Any ideas on how we can bypass these? These are the lines that we need to adjust: |
@moollaza Think we'd need to add module-to-id mapping back to duckpan. When the first lookup fails, derive the id from the module, push it, and it should work for the |
@moollaza someone new on slack ran into this today: https://duckduckhack.slack.com/archives/ask-anything/p1480142217001441 |
@zachthompson this is now ready for further review. I have tested this by creating a new Goodie and Spice without metadata and running Here's a screenshot of a "request headers" Spice that has no Metadata, showing a result: |
…ahir/remove-metadata-requirement * 'master' of github.com:duckduckgo/p5-app-duckpan: Fix handling of nonexistant test Ensure Metadata lookup was successful, exit if not Fix 'no warnings' declaration Check if we're dealing with Fathead regardless of perl_module existance ensure perl_module is defined before using Create HTTP::Headers obj from headers hash Only grab metadata once Add Fathead support to DuckPAN Test
lib/App/DuckPAN/Cmd/Server.pm
Outdated
sub change_css { | ||
my ( $self, $css ) = @_; | ||
my $hostname = $self->hostname; | ||
$css =~ s!:\s*url\((["'])?/!:url\($1http://$hostname/!g; | ||
|
||
$css =~ s|:\s*url\(["']([^'"]+)["']\)|:url\("http://$hostname/$1")|gi; |
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.
This fixes a separate bug I ran into...
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.
Which is what?
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.
The problem was related to our parsing and re-writing of relative urls in the CSS files we pull down from duckduckgo.com.
After looking into this more, I've realized that re-writing the CSS isn't even necessary because Web.pm proxies relative urls to the appropriate upstream host.
Removing the body of this function has no effect on the SERP AFAICT.
I will remove these changes
lib/App/DuckPAN/DDG.pm
Outdated
@@ -74,11 +92,21 @@ sub get_blocks_from_current_dir { | |||
if (my $ia = $self->app->get_ia_by_name($arg)) { | |||
$class = $ia->{perl_module}; | |||
} | |||
elsif ($arg =~ /_/) { |
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.
What about IDs with no underscore?
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.
Good catch 👍
I will check if it matches /^[a-z0-9_]+$/
which resembles a lowercased ID.
If there are uppercase letters we can assume it's a Perl Module Name.
@zachthompson I've revised the logic here I think I've now accounted for any kind of valid input and it now bails on invalid inputs: |
Thanks. |
Background
Devs shouldn't need an IA page in dev status to play with DuckPAN.
With the switch to the Programming Mission, devs are currently force to open PRs to test any new IAs, which means they are required to open PRs for non-mission IAs as well, which bloats the repo with PRs.
We should enable DuckPAN to use temporary metadata in order to trigger local IA's and show results.
This will help keep the repos clean with only PRs for IAs that are real and intended to move forward.
Changes
ID
based on the Perl module/cc @mintsoft