-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[dashboard] Remove Prebuild logs and button from Project Settings #8831
Conversation
313f48b
to
9261a39
Compare
9261a39
to
ca8a204
Compare
@jldec Let me know if this matches your expectation for the prebuild removal. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
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.
Works as advertised!
Just confused with another PR of yours. This is next.
/werft run 👍 started the job as gitpod-build-mp-pb-status-proj-conf.9 |
ca8a204
to
52ff8b9
Compare
68481c9
to
79d05a4
Compare
@gtsiolis @jldec I've now pushed a fix for the issue you've discovered, thanks! I'm moving this back into review. The behaviour is now as follows when there is no
This doesn't feel like the most intuitive, but matches the original behaviour (which was |
Thanks, @easyCZ! Works as before now.
Agree, this is also a bit confusing as we auto-save but not fast enough to include all latest changes in the updated and injected issue: One seemingly important issue but probably not worth blocking merging this is that now when the configuration is invalid we're blocking the users from starting a new workspace without any error or visual feedback as we did with when having the ability to trigger a prebuild, see screenshot below. ❗
|
@easyCZ, I expected the prebuild from the config to show up in the prebuild log view inside the start workspace page.
If @gtsiolis is right about ☝️ then the autosave is not working reliably - pending changes should be saved before the button click takes you out of the page. I don't think we need a separate save button, but it would be nice indicate in the UI that the save has happened. I can live without this enhancement for this PR. See #6724 Bad gitpod.yml errors should also show up on the start workspace page e.g. |
I'm digging into this some more just now. It looks like for this behaviour to remain the same, the following must happen in order:
In the current implementation, we're not triggering the prebuild and as such the workspace start drops you into the IDE with the The fix is to re-add the trigger of a prebuild when you're starting a workspace. However, now with the prebuild view gone, we don't have a place to report progress. How do you want to handle this @jldec? |
41e8b26
to
61b07a9
Compare
I've now updated this PR with the following:
Once staging with new images becomes usable again, I'll put this back into review. |
61b07a9
to
00972cb
Compare
/werft run 👍 started the job as gitpod-build-mp-pb-status-proj-conf.20 |
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.
LGTM
Thanks for starting the cleanup
I think this is good for now - I expect we will remove this dashboard editor feature entirely when we deprecate the config in DB with #9102
/hold for other reviews
Looking at this now! 👀 |
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.
UX LGTM.
Left some minor comments below, some of these probably could be best as follow up issues.
Thanks for the ping, @easyCZ! 🏓
useEffect(() => { | ||
document.title = "Configure Project — Gitpod"; | ||
}, []); | ||
setProgressMessage("Starting a new prebuild..."); |
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.
question: Is there a limitation that does not allow us to simply redirect to the workspace start page which automatically surfaces when a prebuild is running?
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.
In my testing, starting a prebuild was required to actually use the new config. It wouldn't get picked up after the save without the prebuild. However, this may be a red-herring. I need to dig into the config & prebuild flow a bit more to understand why this is, and if in fact this is the case.
Will investigate!
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've attempted to remove the prebuild step but the saved configuration is not reflected in the workspace. I will continue to investigate but in the meantime, this PR is functionally complete and will land.
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.
Sounds good! Thanks for taking a closer look, @easyCZ!
useEffect(() => { | ||
document.title = "Configure Project — Gitpod"; | ||
}, []); | ||
setProgressMessage("Starting a new prebuild..."); |
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.
issue: This could be minor but starting a prebuild is not always the case, right? When the default skeleton configuration is generated for a new project, a prebuild will not actually start, no?
tasks:
- init: |
echo 'TODO: build project'
command: |
echo 'TODO: start app'
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 point! Related to the other point raised around:
question: Is there a limitation that does not allow us to simply redirect to the workspace start page which automatically surfaces when a prebuild is running?
Will investigate!
/unhold |
Description
Removes the following:
Old:
New:
Related Issue(s)
How to test
Release Notes
/hold