Skip to content

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

Merged
merged 14 commits into from
May 6, 2025

Conversation

syn-zhu
Copy link
Contributor

@syn-zhu syn-zhu commented Apr 30, 2025

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

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@syn-zhu syn-zhu changed the title Update atlas cluster states (feat): update atlas cluster states Apr 30, 2025
@syn-zhu syn-zhu changed the title (feat): update atlas cluster states feat: update atlas cluster states Apr 30, 2025
@github-actions github-actions bot added the feat label Apr 30, 2025
@syn-zhu syn-zhu requested a review from Anemy April 30, 2025 02:48
@syn-zhu syn-zhu added the no release notes Fix or feature not for release notes label Apr 30, 2025
@syn-zhu syn-zhu marked this pull request as ready for review April 30, 2025 21:48
@syn-zhu syn-zhu changed the title feat: update atlas cluster states feat: update atlas cluster states COMPASS-8228 May 1, 2025
Copy link
Member

@Anemy Anemy left a 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.

@@ -48,7 +48,7 @@ export interface AtlasClusterMetadata {
| 'UPDATING'
| 'PAUSED'
| 'IDLE'
| 'REPARING'
| 'REPAIRING'
| 'DELETING'
| 'DELETED';
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 20 to 21
const storeRef = useRef(useStore());
const [ref] = useState(() => {
Copy link
Member

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.

Copy link
Contributor Author

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?

@syn-zhu syn-zhu requested a review from Anemy May 2, 2025 14:18
@@ -1399,7 +1427,9 @@ function getDescriptionForNonRetryableError(error: Error): string {
// to the generic error description.
const reason = error.message.match(/code: \d+, reason: (.*)$/)?.[1];
return reason && reason.length > 0
? reason
? reason.endsWith('.')
? reason.slice(0, -1)
Copy link
Member

Choose a reason for hiding this comment

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

TIL negative numbers work with slice

@Anemy
Copy link
Member

Anemy commented May 2, 2025

Err once unit tests are green and we should probably add some tests of our own.

@syn-zhu syn-zhu merged commit b5940fc into main May 6, 2025
25 of 26 checks passed
@syn-zhu syn-zhu deleted the update-atlas-cluster-states branch May 6, 2025 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants