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

INTMDB-299: Added snapshot export bucket support #276

Merged
merged 4 commits into from Feb 9, 2022

Conversation

abner-dou
Copy link
Contributor

Description

Added snapshot export buckets support

Link to any related issue(s): INTMDB-299

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

@abner-dou abner-dou requested a review from a team as a code owner February 1, 2022 20:56
@abner-dou abner-dou requested review from johansteyn and removed request for a team February 1, 2022 20:56
// CloudProviderSnapshotExportBucket represents one cloud provider snapshot export buckets.
type CloudProviderSnapshotExportBucket struct {
ID string `json:"_id,omitempty"` // Unique identifier of the S3 bucket.
ProjectID string `json:"projectId,omitempty"` // Unique 24-hexadecimal digit string identifying the project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q] this is only documented for the create, but even so the example doesn't use it, is this really necessary?

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 can remove it, but I don't know if the end users will need it, due it's in the create documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you have tested the curls and it does actually show or is needed we keep it, I'm just wondering if this is a documentation error, not a blocker to keep

Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@thetonymaster thetonymaster left a comment

Choose a reason for hiding this comment

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

LGTM after Gus comments

@abner-dou
Copy link
Contributor Author

@gssbzn @thetonymaster can you merge this please

@gssbzn gssbzn merged commit daded9c into mongodb:master Feb 9, 2022
@gssbzn
Copy link
Collaborator

gssbzn commented Feb 9, 2022

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants