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

Add Nexus OutgoingService CRUD #370

Merged
merged 4 commits into from
Mar 18, 2024
Merged

Add Nexus OutgoingService CRUD #370

merged 4 commits into from
Mar 18, 2024

Conversation

MichaelSnowden
Copy link
Contributor

What changed?
Add OperatorService APIs for managing Nexus services to which we will send traffic.

Why?

Breaking changes

@MichaelSnowden MichaelSnowden requested review from a team as code owners March 13, 2024 21:44
api-linter.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't some of this in the other PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is meant to be a stacked PR. Should've put that in the description.

Comment on lines +106 to +118
// Create a Nexus service. This will fail if a service with the same name already exists in the namespace with a
// status of ALREADY_EXISTS.
// Returns the updated service with its initial version. You may use this version for subsequent updates. You don't
// need to increment the version yourself. The server will increment the version for you after each update.
rpc CreateNexusOutgoingService(CreateNexusOutgoingServiceRequest) returns (CreateNexusOutgoingServiceResponse) {
}

// Update an outgoing Nexus service by namespace and service name. The version in the request should match the
// current version of the service. This will fail with a status of FAILED_PRECONDITION if the version does not match.
// Returns the updated service with the updated version, which can be used for subsequent updates. You don't need
// to increment the version yourself. The server will increment the version for you.
rpc UpdateNexusOutgoingService(UpdateNexusOutgoingServiceRequest) returns (UpdateNexusOutgoingServiceResponse) {
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional to have separate create/update calls for outgoing but one create-or-update for incoming?

Copy link
Member

Choose a reason for hiding this comment

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

We'll also separate these for incoming in a followup PR.

Copy link
Member

@cretz cretz Mar 14, 2024

Choose a reason for hiding this comment

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

If we know we're going to do it, might as well do it in a common PR IMO. But if coming soon after, ok, though it's a bit hard to approve this PR not knowing what that will look like.

The separate PRs makes it hard to review as a whole. I was lucky enough have looked at the other Nexus APIs from other past PRs for consistency or I would have missed this inconsistency entirely (even though I'm sure y'all knew about it, it was unclear to me).

Copy link
Member

Choose a reason for hiding this comment

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

I'll submit a PR to follow this one once it's merged.

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

The protos LGTM, please separate the linter change into a different PR.

@MichaelSnowden
Copy link
Contributor Author

The protos LGTM, please separate the linter change into a different PR.

Rebased with those on master now.

@MichaelSnowden MichaelSnowden merged commit 905672f into master Mar 18, 2024
3 checks passed
@MichaelSnowden MichaelSnowden deleted the snowden/nexus branch March 18, 2024 22:03
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.

3 participants