Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[AssetMapper] Add logical failure implementation#58275
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedSep 16, 2024
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
carsonbot commentedSep 16, 2024
Hey! Thanks for your PR. You are targeting branch "7.2" but it seems your PR description refers to branch "7.2 for features / 5.4, 6.4, and 7.1 for bug fixes". Cheers! Carsonbot |
src/Symfony/Component/AssetMapper/Command/ImportMapInstallCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
As there is already aLogicException availabe in AssetMapper, maybe we should use it instead of the global one ? For instance (some others in the class / related ones): symfony/src/Symfony/Component/AssetMapper/ImportMap/RemotePackageDownloader.php Lines 74 to 77 inf91514d
I dont think this would be a BC break (cc@stof ?) |
I added additional exceptions to be catch'ed. Reason for this is because our pipeline was failing again with the following error. The added exceptions stopped the script from continuing which is what we need to not break our pipeline. #28 70.49 [2024-09-25T17:41:43.741652+02:00] console.CRITICAL: Error thrown while running command "importmap:install". Message: "Idle timeout reached for "https://cdn.jsdelivr.net/npm/@fortawesome/fontawesome-free@6.5.2/webfonts/fa-solid-900.ttf"." {"exception":"[object] (Symfony\\Component\\HttpClient\\Exception\\TimeoutException(code: 0): Idle timeout reached for \"https://cdn.jsdelivr.net/npm/@fortawesome/fontawesome-free@6.5.2/webfonts/fa-solid-900.ttf\". at /var/www/html/vendor/symfony/http-client/Chunk/ErrorChunk.php:61)","command":"importmap:install","message":"Idle timeout reached for \"https://cdn.jsdelivr.net/npm/@fortawesome/fontawesome-free@6.5.2/webfonts/fa-solid-900.ttf\"."}#28 70.50 17:41:43 CRITICAL [console] Error thrown while running command "importmap:install". Message: "Idle timeout reached for "https://cdn.jsdelivr.net/npm/@fortawesome/fontawesome-free@6.5.2/webfonts/fa-solid-900.ttf"." ["exception" => Symfony\Component\HttpClient\Exception\TimeoutException { …},"command" => "importmap:install","message" => "Idle timeout reached for "https://cdn.jsdelivr.net/npm/@fortawesome/fontawesome-free@6.5.2/webfonts/fa-solid-900.ttf"."] |
src/Symfony/Component/AssetMapper/Command/ImportMapInstallCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Unsure why the tests are failing here. |
| try { | ||
| $contents = $this->packageResolver->downloadPackages($remoteEntriesToDownload, $progressCallback); | ||
| } catch (TimeoutException|TransportException $exception) { | ||
| throw new LogicException($exception->getMessage()); | ||
| } |
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 don't think this is a good idea, as it's hiding the real exception. Also, this is not a "Logic" exception.
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.
Ok, I understand but how would y'all like to see this in that case. It's hard to guess what direction I need to go with this.
Again I'm new to the Symfony world and just want to fix this specific issue because it is breaking deployments. And while I want to fix it I do want to do it while communicating.
Let me know if you have any thoughts on how to improve things. Have a nice evening!
Pure curiosity, is it always the same files that trigger the exceptions ? Does some http client tuning limit the problem ? |
wotta commentedSep 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Great question! So the answer might be a bit useless but it's not always the same. A month or maybe a bit more ago jsdelivr had issues. Not fully sure but I thought that was a certificate issue. That broke the deployments because import map didn't stop. And every now and then the script fails without saying it went wrong because of timeouts, and or issues on jsdelivr's end. It is definitely not consistent but our issue can be easily solved if the import map script just returns the failure signal on error. With on error I mean if anything halts the download process basically. |
Uh oh!
There was an error while loading.Please reload this page.
The following feature/change is to make sure we can rely on the correct status codes in the CI/CD pipelines.
Whenever the download part fail now we get the correct failure status code.
Incase things are missing please let me know, Not sure if any additional information is needed since this seems like a small change which appears to be pretty clear.