-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[projects] always enable New Workspace button on config page #6389
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
Conversation
7ec441b
to
da3574c
Compare
I thought we could just show one button, that would use the I think we should add something like "What's a prebuild?" to this screen. cc @gtsiolis |
as the prebuild context url was subject to be phased out for regular usage, it wasn't considered for prebuilds with the additional configuration, as far as I see. given that, the only path to use the config in a following prebuild is to store it first, trigger a new prebuild, and then start a workspace. |
- 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.
I like what you did here (undetected project yaml using valid echo commands) 🙂 👍
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.
@AlexTugarev, unfortunately the yaml doesn't parse :
inside a string unless the whole value is quoted.
I recommend using the | and putting the echo command like you have it indented on the next line.
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.
nice catch! thanks!
components/dashboard/src/App.tsx
Outdated
@@ -239,7 +239,7 @@ function App() { | |||
<Route path="/admin/workspaces" component={WorkspacesSearch} /> | |||
|
|||
<Route path={["/", "/login"]} exact> | |||
<Redirect to="/workspaces" /> | |||
<Redirect to="/workspaces?blabla" /> |
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.
Why is this ?blabla
here?
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.
oh, that's a leftover. (I had a hard time to convince react router to redirect to /#context_url
and this was a test to see where the redirect broke. then just forgot to remove it. )
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.
:)
Thanks @AlexTugarev This does accomplish the main objective of unblocking users who get stuck with invalid yaml 👍 I do have similar feedback as @svenefftinge. I think new users will still be confused by 2 buttons especially when the less familiar "Run Prebuild" button is the green one. I propose the following small UI changes to address this UI Proposal
|
@gtsiolis wdyt - are you 👍 with this? It seems like the easiest way to reduce confusion about 2 buttons, and also preserve the abililty to do a quick inner-loop edit-run-prebuild.
cc: @svenefftinge |
/werft run 👍 started the job as gitpod-build-at-new-workspace.4 |
Looking at this now! 👀 |
da3574c
to
3accc7f
Compare
/werft run 👍 started the job as gitpod-build-at-new-workspace.6 |
Discussed with @AlexTugarev and @gtsiolis Decided to resolve #6303 with minimal button changes and not remove the logs output from the project configuration page in this PR. Will track that work separately in #5785 |
9c07e42
to
e735b4f
Compare
@jldec, please have a look again. 🙏🏻 |
- 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.
@AlexTugarev, unfortunately the yaml doesn't parse :
inside a string unless the whole value is quoted.
I recommend using the | and putting the echo command like you have it indented on the next line.
tasks:
- init: |
echo 'TODO: build project'
command: |
echo 'TODO: start app'
b.t.w |
@AlexTugarev do you think we could allow the NewWorkspaceButton without waiting for isDetecting? Detection is currently not working for private repos and maybe it might fail for other reasons in future, so allowing the flow to continue to the New Workspace without detection would be better IMO. Alternatively, the detection could block theflow for a 1 or 2 seconds, but then time-out (or err-out) and allow New Workspace to continue. |
e735b4f
to
ca80f70
Compare
LGTM label has been added. Git tree hash: 410513446588bce9969b63355689c46eb76a0d94
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jldec Associated issue: #6303 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I resolved the "requested changes" (yaml fix) which triggered an approved review, and then a merge. |
Playing around with
New Workspace
button to trigger a prebuild and launch a workspace for that.Triggering a prebuild, or waiting for the response seems to add ~2 seconds. I'm not sure if we'll see timing issue if the await would be removed, at least I couldn't force a second prebuild on the same sha+config.
Resolves #6303