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

fix: small api fixes #4627

Merged
merged 8 commits into from
Apr 10, 2024
Merged

fix: small api fixes #4627

merged 8 commits into from
Apr 10, 2024

Conversation

acha-bill
Copy link
Contributor

Description

Small api fixes

closes #4617

@acha-bill acha-bill marked this pull request as ready for review April 3, 2024 11:11
@acha-bill acha-bill requested a review from NoahMaizels April 3, 2024 11:11
Copy link
Contributor

@NoahMaizels NoahMaizels left a comment

Choose a reason for hiding this comment

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

In reference to #4617:

  • Was point 1 addressed? I don't think I see some change here which would address it
  • Seems that all the other points (2 through 6) have been addressed
  • Has there been any discussion already about points 2 and 6? I don't know if changing those might have some sort of unintended side effects?
    • Point 2: Has there been any discussion about renaming peer? We had some uncertainty on that one.
    • Point 6: Has there been any discussion about changing the neighborhoodSize to be inclusive of all nodes in the neighborhood?

@acha-bill acha-bill requested a review from janos April 8, 2024 17:07
@@ -1,6 +1,6 @@
openapi: 3.0.3
info:
version: 3.2.7
version: 3.2.8
Copy link
Member

Choose a reason for hiding this comment

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

The API has breaking changes (renaming two fields) and the version is only updating the bugfix number. To be semantically correct, API number should be raised to 4, or to keep old fields for backward compatibility. It depends on the expectations for the API stability, but since the major version is not 0, the API supposed to be stable.

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

The changes are ok, it is just a question are backward compatibility changes are needed or the API can be incompatible at these two fields.

@acha-bill
Copy link
Contributor Author

Let's leave this breaking change and update the API version then.
BTW, we are going to be adding more API breaking changes in the next release with the merge of debug and bee APIs. So this can be seen as a first step.

wdyt @NoahMaizels ?

@NoahMaizels NoahMaizels self-requested a review April 9, 2024 11:08
@NoahMaizels
Copy link
Contributor

Let's leave this breaking change and update the API version then. BTW, we are going to be adding more API breaking changes in the next release with the merge of debug and bee APIs. So this can be seen as a first step.

wdyt @NoahMaizels ?

Yeah I agree with you, I think it can be included in the next release since like you said it will be including breaking changes as well and we already have plans to communicate about those in advance

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Thanks

@acha-bill acha-bill changed the base branch from master to feat/api-merge April 10, 2024 09:22
@acha-bill acha-bill merged commit 170d484 into feat/api-merge Apr 10, 2024
11 of 14 checks passed
@acha-bill acha-bill deleted the api-fixes branch April 10, 2024 09:23
acha-bill added a commit that referenced this pull request Apr 11, 2024
acha-bill added a commit that referenced this pull request Apr 17, 2024
acha-bill added a commit that referenced this pull request Apr 19, 2024
acha-bill added a commit that referenced this pull request Apr 25, 2024
acha-bill added a commit that referenced this pull request Apr 25, 2024
@acha-bill acha-bill mentioned this pull request Jun 11, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple Small API Fixes
3 participants