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

rbd: Add mirror peer site API components #850

Merged
merged 1 commit into from
Mar 27, 2023
Merged

rbd: Add mirror peer site API components #850

merged 1 commit into from
Mar 27, 2023

Conversation

Nikhil-Ladha
Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha commented Mar 23, 2023 •
edited
Loading

Description:
Added some API components of mirror peer site

Fixes: #486

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

@anoopcs9
Copy link
Collaborator

Thanks for your contribution.

As a general note, go-ceph still expects new APIs and corresponding tests in separate source files(for example, mirror_peer_site.go and mirror_peer_site_test.go]. This is done to ensure that we keep track of such new APIs in preview state for 2 releases(by default) before promoting to stable state. In addition it will help us to mention those APIs in api-status.md. You may go through API Status section from Developer's Guide and API Stability doc for more details.

@Nikhil-Ladha
Copy link
Contributor Author

Thanks for your contribution.

As a general note, go-ceph still expects new APIs and corresponding tests in separate source files(for example, mirror_peer_site.go and mirror_peer_site_test.go]. This is done to ensure that we keep track of such new APIs in preview state for 2 releases(by default) before promoting to stable state. In addition it will help us to mention those APIs in api-status.md. You may go through API Status section from Developer's Guide and API Stability doc for more details.

I was about to confirm this part, thanks for the note. Will update the code accordingly.

@Nikhil-Ladha
rbd: Add mirror peer site API components
609903e 
**Description:**
Added some API components of mirror peer site

Fixes: ceph#486

Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
@Nikhil-Ladha
Copy link
Contributor Author

@anoopcs9 PTAL now, and could you please also retrigger the quincy test suite?

@phlogistonjohn
Copy link
Collaborator

Wow, very impressive contribution thanks! Docs look OK so I can suggest you tick off that item in the checklist. We hit two ci "flakes" but (I hope) we just got unlucky. I'll do a proper full review once we have the CI passing!

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Mar 23, 2023
@Nikhil-Ladha
Copy link
Contributor Author

Wow, very impressive contribution thanks! Docs look OK so I can suggest you tick off that item in the checklist. We hit two ci "flakes" but (I hope) we just got unlucky. I'll do a proper full review once we have the CI passing!

Seems like it was actually flaky 😅

phlogistonjohn
phlogistonjohn approved these changes Mar 23, 2023
View reviewed changes
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

LGTM!

@phlogistonjohn
Copy link
Collaborator

Seems like it was actually flaky sweat_smile

Indeed. There's a common-ish flake in go-ceph where the test suite runs and reports "FAIL" at the end but if you look back through the output there's no indication of a particular failed test. I have yet to find out a cause and I have only ever seen it happen on the github ci, never when running tests locally.

@Nikhil-Ladha
Copy link
Contributor Author

If everything looks fine, are we good to merge the PR?

anoopcs9
anoopcs9 approved these changes Mar 27, 2023
View reviewed changes
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@mergify mergify bot merged commit fcd2fcd into ceph:master Mar 27, 2023
@Nikhil-Ladha Nikhil-Ladha deleted the issue486/add_rbd_apis branch March 28, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anoopcs9 anoopcs9 anoopcs9 approved these changes

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

Assignees
No one assigned
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing rbd API components: mirror peer site
3 participants
@Nikhil-Ladha @anoopcs9 @phlogistonjohn