Skip to content
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

Do not show modal for trusted repositories #1172

Merged
merged 9 commits into from
Aug 30, 2024
Merged

Conversation

akurinnoy
Copy link
Contributor

@akurinnoy akurinnoy commented Aug 21, 2024

What does this PR do?

This PR fixes

  • the behavior of the Untrusted Repository modal, so that it doesn't reappear for already trusted repositories (see "Bug # 1);
  • when switching to either the Logs or Events tab during the workspace creation phase, and staying there for a while, then the warning about the limit of running workspaces appears (see "Bug # 2"). The workaround for this bug is to disable the Logs and Events tab until the workspace is fully created;
  • do not show the Untrusted Repository modal when starting an existing workspaces;
  • do not show the Untrusted Repository modal when creating a workspace from the sample project on the Getting Started page.

Screenshot/screencast of this PR

Bug # 1
Screen.Recording.2024-08-21.at.13.15.53.mov
Bug # 2
Screen.Recording.2024-08-21.at.13.41.02.mov
With fixes
Screen.Recording.2024-08-21.at.13.23.35.mov

What issues does this PR fix or reference?

fixes eclipse-che/che#23097
fixes eclipse-che/che#23107
fixes eclipse-che/che#23106
fixes CRW-7139

Is it tested? How?

See the screencasts.

Release Notes

Docs PR

@akurinnoy akurinnoy self-assigned this Aug 21, 2024
@che-bot
Copy link
Contributor

che-bot commented Aug 21, 2024

Click here to review and test in web IDE: Contribute

@@ -10,6 +10,8 @@

FROM docker.io/node:18.19.1-alpine3.19

LABEL quay.expires-after=1w
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use this docker file to build images for testing purposes, so we don't need them to be in the registry forever.

@ibuziuk ibuziuk requested a review from dmytro-ndp August 21, 2024 13:46
@ibuziuk
Copy link
Member

ibuziuk commented Aug 21, 2024

great follow up PR, should be backported to 7.90.x for 3.16 cc: @dmytro-ndp @artaleks9

Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1172

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1172", name: che-dashboard}]}}]"

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 93.90244% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.08%. Comparing base (2ae50bc) to head (b7824a6).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...-frontend/src/store/DevfileRegistries/selectors.ts 78.57% 3 Missing ⚠️
...paceProgress/CreatingSteps/Apply/Devfile/index.tsx 75.00% 1 Missing ⚠️
...ceProgress/CreatingSteps/Apply/Resources/index.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1172   +/-   ##
=======================================
  Coverage   90.08%   90.08%           
=======================================
  Files         432      432           
  Lines       44718    44763   +45     
  Branches     3078     3095   +17     
=======================================
+ Hits        40282    40323   +41     
- Misses       4399     4402    +3     
- Partials       37       38    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1172

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1172", name: che-dashboard}]}}]"

@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Aug 27, 2024

@akurinnoy : thanks for the fixup!

Validation results on Dev Spaces 3.16.0.RC + quay.io/eclipse/che-dashboard:pr-1172

1) The behavior of the Untrusted Repository modal, so that it doesn't reappear for already trusted repositories
2) when switching to either the Logs or Events tab during the workspace creation phase, and staying there for a while, then the warning about the limit of running workspaces appears (see "Bug # 2"). The workaround for this bug is to disable the Logs and Events tab until the workspace is fully created;

worked as expected:

Untrusted.Repository.modal.reappears.when.the.workspace.starts.webm

3) Do not show the Untrusted Repository modal when starting an existing workspaces

worked as expected:

test_start_existed_workspace.webm

issue eclipse-che/che#23097

couldn't reproduce

Things to address

Storing of trusted sources seems to be broken, and Untrusted Author popup appears again when create new workspace from the same factory URL after the clicking on the Continue button previously:

popup.appears.again.when.create.new.workspace.from.the.same.factory.URL.webm

4) do not show the Untrusted Repository modal when creating a workspace from the sample project on the Getting Started page

Need help to configure Che Dashboard to Dev Spaces specific samples in Dashboard
Screenshot from 2024-08-27 19-40-01

Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci openshift-ci bot removed the lgtm label Aug 29, 2024
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1172

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1172", name: che-dashboard}]}}]"

@dmytro-ndp
Copy link
Contributor

@akurinnoy : I have retested new image from this PR: quay.io/eclipse/che-dashboard@sha256:6192db43235b5f1b729e5e8ea5c72d5793d3a527abdf907e488d4ecfc1d9365a

In the updated User Dashboard the Untrusted Author popup didn't appear again when create new workspace from the same factory URL.
Well done!

I still need help with verification of the next fix:

4) do not show the Untrusted Repository modal when creating a workspace from the sample project on the Getting Started page

Provided Dev Spaces configuration didn't work.

Copy link
Contributor

@dmytro-ndp dmytro-ndp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akurinnoy : I have managed to test it with Dev Spaces 3.16.0.RC using quay.io/abazko/che-dashboard:CRW-7139 which is quay.io/eclipse/che-dashboard:pr-1172 adapted to run in Dev Spaces by @tolusha .

No issues mentioned in the description of PR have been reproduced:

che-dashboard_pull_1172_with_devspaces.webm

Well done!

Copy link

openshift-ci bot commented Aug 30, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dmytro-ndp, olexii4

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@akurinnoy
Copy link
Contributor Author

@dmytro-ndp, Thank you!

@akurinnoy akurinnoy merged commit ca29986 into main Aug 30, 2024
18 checks passed
@akurinnoy akurinnoy deleted the unexpected-untrusted-modal branch August 30, 2024 12:40
akurinnoy added a commit that referenced this pull request Aug 30, 2024
* fix: unexpected modal for untrusted repo

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fix: disable the Logs and Events tabs until the workspace is created

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* chore: set dockerimage tag expiration date

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fixup! fix: unexpected modal for untrusted repo

* fixup! fix: disable the Logs and Events tabs until the workspace is created

* fix: configmap samples are trusted

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fix: two browser tabs when starting a factory flow

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fixup! fix: two browser tabs when starting a factory flow

* fix: unexpected modal for trusted source

Signed-off-by: Oleksii Kurinnyi <[email protected]>

---------

Signed-off-by: Oleksii Kurinnyi <[email protected]>
dmytro-ndp pushed a commit that referenced this pull request Sep 2, 2024
* Do not show modal for trusted repositories (#1172)

* fix: unexpected modal for untrusted repo

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fix: disable the Logs and Events tabs until the workspace is created

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* chore: set dockerimage tag expiration date

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fixup! fix: unexpected modal for untrusted repo

* fixup! fix: disable the Logs and Events tabs until the workspace is created

* fix: configmap samples are trusted

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fix: two browser tabs when starting a factory flow

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fixup! fix: two browser tabs when starting a factory flow

* fix: unexpected modal for trusted source

Signed-off-by: Oleksii Kurinnyi <[email protected]>

---------

Signed-off-by: Oleksii Kurinnyi <[email protected]>

* fix: untrusted modal

Signed-off-by: Oleksii Kurinnyi <[email protected]>

---------

Signed-off-by: Oleksii Kurinnyi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants