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

Gutenberg PR Preview: Broken check for the build artifact availability #1568

Open
WunderBart opened this issue Jul 2, 2024 · 3 comments
Open
Assignees
Labels
[Feature] GitHub integration [Type] Bug An existing feature does not function as intended

Comments

@WunderBart
Copy link
Member

WunderBart commented Jul 2, 2024

The endpoint for checking the zipped build availability returns a different payload depending on the artifact availability, but it's always a 200:

Request Artifact unavailable Artifact available
Code 200 200
Payload JSON: { "error": "Request failed" } zip file

The code that validates the artifact availability checks only whether it's a 200 or not, meaning it always passes that check. The consequence is that a wrong error is thrown, which can be misleading:

JavaScript Error: Error when executing the blueprint step #2 ({"step":"writeFile","path":"/wordpress/pr/pr.zip","data":{"resource":"url","url":"/plugin-proxy.php?org=WordPress&repo=gutenberg&workflow=Build%20Gutenberg%20Plugin%20Zip&artifact=gutenberg-plugin&pr=59242","caption":"Downloading Gutenberg PR 59242"},"progress":{"weight":2,"caption":"Applying Gutenberg PR 59242"}}) : Could not download "/plugin-proxy.php?org=WordPress&repo=gutenberg&workflow=Build%20Gutenberg%20Plugin%20Zip&artifact=gutenberg-plugin&pr=59242".
Check if the URL is correct and the server is reachable.
If it is reachable, the server might be blocking the request.

The correct error thrown should be:

The PR 12345 does not exist or GitHub CI did not finish building it yet.
@bgrgicak bgrgicak moved this from Inbox to Needs Triage/Our Reply in Playground Board Jul 5, 2024
@bgrgicak bgrgicak added [Type] Bug An existing feature does not function as intended [Feature] GitHub integration labels Jul 5, 2024
@bgrgicak bgrgicak moved this from Needs Triage/Our Reply to Up next in Playground Board Jul 5, 2024
@bgrgicak bgrgicak self-assigned this Jul 22, 2024
@bgrgicak bgrgicak moved this from Up next to In progress in Playground Board Jul 22, 2024
@bgrgicak
Copy link
Collaborator

@brandonpayton I could use your help with this one because it looks like a server issue.

Production

I get a 200 and an empty plugin-proxy.php is downloaded.

https://playground.wordpress.net/plugin-proxy.php?org=WordPress&repo=gutenberg&workflow=Build%20Gutenberg%20Plugin%20Zip&artifact=gutenberg-plugin&pr=59242&verify_only=true

Locally

Start a server:

cd packages/playground/website/public/
php -S localhost:4321 -d .

I get a 400 error as expected.

http://localhost:4321/plugin-proxy.php?org=WordPress&repo=gutenberg&workflow=Build%20Gutenberg%20Plugin%20Zip&artifact=gutenberg-plugin&pr=59242&verify_only=true

@bgrgicak bgrgicak assigned brandonpayton and unassigned bgrgicak Jul 22, 2024
@adamziel
Copy link
Collaborator

@bgrgicak are you setting GITHUB_TOKEN locally?

@adamziel
Copy link
Collaborator

The problem here is that the /plugin-proxy.php?verify_only=yes request short-circuits with HTTP 200 when it finds an artifact:

if (array_key_exists('verify_only', $_GET)) {

Whereas the /plugin-proxy.php request tries to fetch that artifact and, in doing so, may fail if the actual file is not available.

This diff seems to solve it:

diff --git a/packages/playground/website/public/plugin-proxy.php b/packages/playground/website/public/plugin-proxy.php
index 59f9522afb..b1a55216a4 100644
--- a/packages/playground/website/public/plugin-proxy.php
+++ b/packages/playground/website/public/plugin-proxy.php
@@ -87,16 +87,6 @@ class PluginDownloader
                 continue;
             }
 
-            /*
-             * Short-circuit with HTTP 200 OK when we only want to
-             * verify whether the CI artifact seems to exist but we
-             * don't want to download it yet.
-             */
-            if (array_key_exists('verify_only', $_GET)) {
-                header('HTTP/1.1 200 OK');
-                return;
-            }
-
             $allowed_headers = array(
                 'content-length',
                 'content-disposition',
@@ -112,6 +102,16 @@ class PluginDownloader
             ob_end_flush();
             flush();
 
+            /*
+             * Short-circuit with HTTP 200 OK when we only want to
+             * verify whether the CI artifact seems to exist but we
+             * don't want to download it yet.
+             */
+            if (array_key_exists('verify_only', $_GET)) {
+                header('HTTP/1.1 200 OK');
+                return;
+            }
+
             // The API endpoint returns the actual artifact URL as a 302 Location header.
             foreach ($artifact_res['headers'] as $header_line) {
                 $header_name = strtolower(substr($header_line, 0, strpos($header_line, ':')));

Any takes to make it into a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] GitHub integration [Type] Bug An existing feature does not function as intended
Projects
Status: Backlog
Development

No branches or pull requests

4 participants