-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Hide the latest editor warning on stopping/stopped phases of a workspace #16940
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
Hide the latest editor warning on stopping/stopped phases of a workspace #16940
Conversation
annotations in the pull request changed, but user is not allowed to start a job |
Some context 💡 - This PR is created as per @mustard-mh's comment in #16551 to hide this warning for the workspace |
Hey, @Devansu-Yadav Thank you for your follow-up contribution 🙏 I'd like to suggest the following conditions to show/hide this alert to close the issue #11035
The issue description #11035 has some code refers. Please let me know if you have any questions or suggestions. |
a2e0435
to
2a1a78b
Compare
@mustard-mh I have pushed a change to address this. Could you please have a look and let me know if it's the correct way to do it?
I think the very first change in this PR should address this. We now hide the alert on |
phase={phase} | ||
error={error} | ||
title={title} | ||
showLatestIdeWarning={isError || (!isTimedOut && !isStoppingOrStopped && useLatest)} |
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.
showLatestIdeWarning={isError || (!isTimedOut && !isStoppingOrStopped && useLatest)} | |
showLatestIdeWarning={useLatest && (isError || (!isTimeout && !isStoppingOrStopped))} |
But it would be good to name isTimeout
and isStoppingOrStopped
together (I can't find a proper name for it XD).
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.
@mustard-mh Do you mean we should combine both the variables isTimeout
and isStoppingOrStopped
?
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.
@mustard-mh Actually, we can just remove the isTimeout
variable entirely since it's just a special case under the Stopping
phase, and we anyways want to hide this alert for any phases after Running
. We could keep the isStoppingorStopped
variable as is.
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.
@mustard-mh Could you help or confirm if what @Devansu-Yadav is suggesting above is ok? 🏓
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.
mustard-mh Could you help or confirm if what Devansu-Yadav is suggesting above is ok? 🏓
From the code, yes 👍
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, @mustard-mh! Let me loop in @gitpod-io/engineering-webapp to bring this up to the team attention in case this could be ready to merge.
/werft run 👍 started the job as gitpod-build-hide-editor-warning-fork.0 |
started the job as gitpod-build-hide-editor-warning.0 because the annotations in the pull request description changed |
2a1a78b
to
4489026
Compare
Deployed a preview environment in https://hide-editor-warning.preview.gitpod-dev.com/workspaces. |
435e26c
to
7489e49
Compare
7489e49
to
e0dbe89
Compare
@gtsiolis Any updates on this PR? |
@Devansu-Yadav Let me loop in @gitpod-io/engineering-webapp to take a closer look at the recent[1] code changes. |
e0dbe89
to
611cfea
Compare
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. |
@gtsiolis Could you pls re-open this PR?
Seems like there hasn't been any update since this 😅 |
Sure, @Devansu-Yadav. Thanks for hanging here! 🙏 Let me re-loop in @gitpod-io/engineering-webapp. |
522dfd0
to
7a0e999
Compare
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. |
@gtsiolis Do you know what is blocking it? |
@akosyakov Unless code changes are no longer applicable, changes should be good to merge since @mustard-mh reviewed this a while ago, see #16940 (comment). 🏓 The problem with merging this is that we no longer can merge community contributions directly from PRs, see relevant section (internal) from our handbook:
Let me merge this in a branch and reopen. ➰ |
…6940) (#18336) Co-authored-by: Devansu Yadav <[email protected]>
Description
Related Issue(s)
Fixes #11035
How to test
Release Notes
Documentation
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh