Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Remove state related to a cluster when removing it #755

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Apr 21, 2022

Closes: https://github.com/gravitational/webapps.e/issues/154
And closes point 1. from https://github.com/gravitational/webapps.e/issues/240

I simplified previous state restoring as we discussed a few days ago (it allowed to fix a bug with restoring previous state after logout and logging back in).

workspaces: {},
},
};
return this._fileStorage.get('state') || defaultState;
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 can get rid of the local state as this call doesn't cause reading from a disk (fileStorage keeps data in memory).

@gzdunek gzdunek requested a review from ravicious April 21, 2022 15:30
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Reset my app state, clicked around and everything seems to work. When I remove a cluster and then log to it again, I'm no longer asked about restoring tabs (idk if that was still the case for the current master version).

@@ -102,6 +102,12 @@ export class ConnectionTrackerService extends ImmutableStore<ConnectionTrackerSt

private _refreshState = () => {
this.setState(draft => {
// filter out connections from removed clusters
draft.connections = draft.connections.filter(i => {
const uri = i.kind === 'connection.gateway' ? i.targetUri : i.serverUri;
Copy link
Member

Choose a reason for hiding this comment

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

This won't scale well once we start adding more connection types, but for now I guess we can go with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware of this, but it can be easily changed to sth like switch-case

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more that the URI should be under the same field across all types of connections so that we don't need any special conditionals here.

gwConn.connected = !!this._clusterService.findGateway(
doc.gatewayUri
);
if (doc.port && doc.targetUri) {
Copy link
Member

Choose a reason for hiding this comment

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

Something like

if (!doc.port || !doc.targetUri) {
   break;
}

would help avoid adding another level of nesting.

BTW, when is targetUri not present here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
I will remove targetUri from the condition - it's a bit confusing as targetUri is always present.

@gzdunek gzdunek force-pushed the gzdunek/remove-cluster-resources-when-removing-it branch from bfd0704 to 535f1da Compare April 25, 2022 16:19
@gzdunek
Copy link
Contributor Author

gzdunek commented Apr 25, 2022

@ravicious I've just found a bug in connectionTracker. When you create a DB connection, then you close the tab and try to reopen it using the tracker, you will get the following error:
image
It crashes, because we don't use gatewayUri from the saved connection and useDocumentGateway tries to create a new gateway even when you have it. I pushed fix for that.

@gzdunek gzdunek force-pushed the gzdunek/remove-cluster-resources-when-removing-it branch from 535f1da to e09eb75 Compare April 26, 2022 08:52
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Great catch with the gateway URI. I totally forgot about the connection tracker when working on that initial support for using CLI command from tsh daemon.

@gzdunek gzdunek merged commit 94955bd into master Apr 26, 2022
@gzdunek gzdunek deleted the gzdunek/remove-cluster-resources-when-removing-it branch April 26, 2022 09:18
ravicious added a commit that referenced this pull request Apr 27, 2022
* Bring back native scrollbar as the styled one causes content to jump when it becomes visible

* Use new colors for theme

* Fix not clickable notifications when displayed over xterm

* Close `Identity` popover after selecting an option (#741)

* Fix getting cwd in presence of lsof warnings (#745)

lsof uses stderr to print out warnings. This caused the previous version
of ptyProcess to throw an error, even though the exit code of lsof was 0.

The existing code already handles non-zero exit codes (asyncExec will
reject the promise). Since we don't know why stderr was inspected in
the first place, let's just remove the check for it.

* Change connections shortcut to `Command/Ctrl-P` (#747)

* Resolve issues on logout (#740)

* Change app name to `Teleport Connect` (#753)

* Show database username suggestions in Teleport Connect (#754)

* Move useAsync to the shared package, add docs

* Add async variant of MenuLogin

* Add Teleport Connect version of MenuLogin to its story

* Update gRPC files

* Show database username suggestions

* Use JSdoc in useAsync

Co-authored-by: Grzegorz Zdunek <[email protected]>

* Convert useAsync to a named export

* Refactor MenuLogin to use useAsync underneath

* Rewrite useAsync to use a promise instead of async

The Web UI currently doesn't support `async`. To do this, we'd need to
configure polyfills there, but we don't have time for that right now.

* Replace async with promise in MenuLogin

Co-authored-by: Grzegorz Zdunek <[email protected]>

* Fix check for the --insecure flag (#758)

Fixes gravitational/webapps.e#147.

* Remove state related to a cluster when removing it (#755)

* Teleport Connect: Add dropdown for database name (#757)

* Add required prop to MenuLogin

This will be needed for the db name dropdown, where the value is optional.

* Update proto files

* Include targetSubresourceName when creating a gateway

* Set better title for gateway tab

* Fix long document titles breaking connection tracker's UI

Co-authored-by: Grzegorz Zdunek <[email protected]>
Co-authored-by: Grzegorz Zdunek <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants