-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Prebuild issues roundup #20000
Prebuild issues roundup #20000
Conversation
f11980f
to
244ea1d
Compare
…nges, and uses the new/old streaming logic WIP because still has the "duplicate (sometimes triple!) logs" react re-rendering issue
244ea1d
to
857236e
Compare
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
Will review and test this PR later 🌮 |
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.
It's all working well in general, and we can do the rest in follow up.
1️⃣ If a prebuild has image-build, and image-build failed, prebuild detail page will stuck in Pending
phase and no updates until user refreshes the page -> Turn into Error
, message Error: headless task failed: exit status 1
and labels Duration
and Stopped
(time) shown
Example context https://github.com/mustard-mh/test/tree/prebuild/image-build-failed
Stuck in Pending |
Refresh Page |
---|---|
2️⃣ nit: Click button to re-run prebuild, it will show Image build
task tab then disappear (should be a follow up?)
3️⃣ Sometimes stopped (watch until it stopped) prebuild detail page doesn't show I could not reproduce it anymoreStopped
and Duration
labels on top right until page refreshed
4️⃣ nit: as we will show commit SHA on prebuild detail page, we could also show such label on prebuilds list page?
5️⃣ Before image build workspace created and recorded in db, dashboard will show inaccessible
content for a long time, while as an user, I don't know if it's really running or pending or never able to access that logs
6️⃣ Re-run prebuild will use the latest commit instead of that commit SHA shows on prebuild detail page (SHA changed)
@mustard-mh many thanks for the review!
I created ENT-445 for this, nice find!
This boils down to us being unable to know whether an image build is going to happen from the shape we get from the API. Not sure if there's any quick wins to be had here.
Interestingly enough, I ran into this as well, but when I tried to show it to @geropl this morning, it didn't happen 😆. I think it means there is for sure an issue, but I could not pin it down yet. Edit: ENT-450 exists now for this
It used to be like this, but I removed it from the list because it took a lot of space in a table where we don't have much of it available.
This should most likely be dealt with separately, as this PR does not change this behavior. I agree we should make this part more reliable, but it might not be worth the effort.
I think this is fine as:
|
This reverts commit fbc0d76.
This reverts commit bb45446.
Description
Hey @filiptronicek 👋
This PR was meant to address a lot of the current prebuild issues, but I did not mange to finish it last week. 😢
The key piece missing is that we render the log output 2 or 3 times now, so a react invalidation problem:
PrebuildLogs.tsx
)Anyway, happy to sync for ~30mins a slot tomorrow morning if that helps. Also, happy to do the review! 🥳
Related Issue(s)
Meant to:
prebuildId
changed - now the keys are "prebuildId + taskId")How to test
https://gpl-prebuild-cleanup.preview.gitpod-dev.com/workspaces
Documentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh
. If enabled,with-preview
andwith-large-vm
will be enabled./hold