-
Notifications
You must be signed in to change notification settings - Fork 212
feat: update atlas cluster states COMPASS-8228 #6884
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 2a14569.
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.
Changes look good, I like the hook and how we're managing this. Left a couple questions, mostly from me not being too familiar with the different cluster states.
</div> | ||
)} | ||
> | ||
<Body>Unpause your cluster to connect to it</Body> |
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.
Have we run this text by product? Some alternative wording (I think I prefer what you already have here)
<Body>Unpause your cluster to connect to it</Body> | |
<Body>A paused cluster cannot be connected to</Body> |
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.
Yeah, this was actually the wording we decided on: https://mongodb.slack.com/archives/C03U2M8C2KC/p1745587663454049?thread_ts=1745248986.165519&cid=C03U2M8C2KC
@@ -291,11 +294,8 @@ export class AtlasCloudConnectionStorage | |||
return connectionInfoList | |||
.map((connectionInfo: ConnectionInfo): ConnectionInfo | null => { | |||
if ( | |||
!connectionInfo.connectionOptions.connectionString || |
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.
When would this happen? Is this related to these changes?
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.
Yeah, so when a cluster is CREATING, it might not have connection strings yet. Previously, this would be one of the states that doesn't show up in Compass Web
'UPDATING', | ||
'PAUSED', | ||
'CREATING', | ||
'DELETING', |
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.
Are these all of the VISIBLE_CLUSTER_STATES
? I'm wondering if that name doesn't fully describe it, as a cluster in the normal/running state isn't one of those states. Is that SUCCESS
or is that IDLE
?
Should DELETED
be here or is that something we want to intentionally omit?
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.
That's IDLE, actually. Every state that should be visible in Compass Web is captured in this list, EXCEPT DELETED.
Let me add DELETED to this.
@@ -48,7 +48,7 @@ export interface AtlasClusterMetadata { | |||
| 'UPDATING' | |||
| 'PAUSED' | |||
| 'IDLE' | |||
| 'REPARING' | |||
| 'REPAIRING' | |||
| 'DELETING' | |||
| 'DELETED'; |
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.
There's a related comment above, is SUCCESSor WORKING a state we want to have here? Where's the source of truth for these?
It's not this right?
https://github.com/10gen/mms/blob/8f63b6c6e79f50a310a662260131f2f05818cdd8/server/src/main/com/xgen/cloud/nds/project/_public/model/ClusterDescription.java#L4681
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.
const storeRef = useRef(useStore()); | ||
const [ref] = useState(() => { |
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.
Would this be a cleaner way to write this?
const store = useStore();
const getConnectable = useCallback((connectionId: string) => {
// ...
}, [store]);
Fine as is too I think.
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.
Ah, would that have the same outcome?
https://jira.mongodb.org/browse/COMPASS-8228
Description
Screen.Recording.2025-04-30.at.5.30.46.PM.mov
Screen.Recording.2025-04-30.at.5.33.21.PM.mov
Screen.Recording.2025-04-30.at.5.44.29.PM.mov
Checklist
New tests and/or benchmarks are included
Documentation is changed or added
If this change updates the UI, screenshots/videos are added and a design review is requested
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)
Motivation and Context
Open Questions
Dependents
Types of changes