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.

Sign up for GitHub

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

Jump to bottom

feat: Adds acceptDataRisksAndForceReplicaSetReconfig parameter in cluster and advanced cluster #518

Merged
merged 2 commits into from Oct 31, 2023
Merged

feat: Adds acceptDataRisksAndForceReplicaSetReconfig parameter in cluster and advanced cluster #518

merged 2 commits into from Oct 31, 2023

Conversation

lantoli
Copy link
Member

@lantoli lantoli commented Oct 30, 2023

Description

Adds acceptDataRisksAndForceReplicaSetReconfig parameter in cluster and advanced cluster.

Ticket: INTMDB-1236

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Required Checklist:

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

@lantoli lantoli requested a review from a team as a code owner October 30, 2023 12:53
@lantoli lantoli marked this pull request as draft October 30, 2023 13:09
wtrocki
wtrocki reviewed Oct 30, 2023
View reviewed changes
mongodbatlas/advanced_clusters.go
@@ -44,27 +44,28 @@ var _ AdvancedClustersService = &AdvancedClustersServiceOp{}

// AdvancedCluster represents MongoDB cluster.
type AdvancedCluster struct {
BackupEnabled *bool `json:"backupEnabled,omitempty"`
BiConnector *BiConnector `json:"biConnector,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid formatter changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that can be done, it's go fmt because the new field name length is larger, I don't think we should override go fmt

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should override go fmt

Apologies for lack of clarity. That wasn't my intention.

I think a good pattern in this case is to add this field as the last element. Since this field is much longer than everything else we can add comment or enter and then rest of the content is not affected:

Screenshot 2023-10-30 at 15 43 05

Hope that makes sense

Copy link
Member

Choose a reason for hiding this comment

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

What I have done:

  1. git reset --soft head~2
  2. Moved field to the bottom
  3. make fmt

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this client we like to keep fields alphabetically ordered for easy comparison in case of missing fields

Copy link
Member Author

@lantoli lantoli Oct 30, 2023 •
edited

Choose a reason for hiding this comment

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

sorry I just saw this comment. Exactly, I wanted to keep the same order as in the new SDK

@wtrocki
Copy link
Member

wtrocki commented Oct 30, 2023

Check: V2 sdk contains the same field: https://github.com/mongodb/atlas-sdk-go/blob/bea1252c35376028cac30983cfb83732f4812472/docs/docs/AdvancedClusterDescription.md?plain=1#L63

@wtrocki
Copy link
Member

wtrocki commented Oct 30, 2023

Good job! will approve after formatting issues are fixed

@lantoli
Copy link
Member Author

lantoli commented Oct 30, 2023 •
edited

Check: V2 sdk contains the same field: https://github.com/mongodb/atlas-sdk-go/blob/bea1252c35376028cac30983cfb83732f4812472/docs/docs/AdvancedClusterDescription.md?plain=1#L63

@wtrocki I know but we've decided in this case to wait until we migrate to new SDK to mitigate risk and fix this soon

@lantoli lantoli marked this pull request as ready for review October 30, 2023 13:29
@lantoli lantoli requested a review from wtrocki October 30, 2023 13:29
@wtrocki
Copy link
Member

wtrocki commented Oct 30, 2023

I know but we've decided in this case to wait until we migrate to new SDK to mitigate risk and fix this soon

Understood. My intention was only to ensure that the name of the property is the same and we have parity so we do not create a blocker for future migration.

wtrocki
wtrocki approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

LGTM. No blockers - formatting can be improved but not blocking

@lantoli
Copy link
Member Author

lantoli commented Oct 30, 2023

thanks @wtrocki , I you don't mind then I'll leave it as it is because I wanted the field to be the first one exactly as in the new SDK (because they're alphabetically sorted)

@wtrocki
Copy link
Member

wtrocki commented Oct 30, 2023

LGTM

@lantoli lantoli merged commit ab5f7e9 into master Oct 31, 2023
3 checks passed
@lantoli lantoli deleted the INTMDB-1236_reconfig branch October 31, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gssbzn gssbzn gssbzn left review comments

@wtrocki wtrocki wtrocki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lantoli @wtrocki @gssbzn