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

upcoming: [M3-9422] -Update existing API endpoints and types for /v4/nodebalancers #11811

Merged

Conversation

harsh-akamai
Copy link
Contributor

Description 📝

Updates existing /v4/nodebalancers endpoints for NB-VPC Integration

How to test 🧪

  • confirm changes match API spec
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@harsh-akamai harsh-akamai self-assigned this Mar 10, 2025
@harsh-akamai harsh-akamai marked this pull request as ready for review March 11, 2025 06:51
@harsh-akamai harsh-akamai requested a review from a team as a code owner March 11, 2025 06:51
@harsh-akamai harsh-akamai requested review from cpathipa and pmakode-akamai and removed request for a team March 11, 2025 06:51
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

This PR is technically M3-9421 and M3-9422, right?

We don't currently have or use the /v4/nodebalancers/{nodeBalancerId}/configs/{configId}/rebuild endpoint, but I think it'd be worth adding the non-beta and beta versions.

I know you mentioned the /v4/vpcs endpoint work was still pending on the back-end -- is that inclusive of the /v4/nodebalancers/{id}/vpcs ones?

@harsh-akamai harsh-akamai changed the title upcoming: [M3-9422] - Update existing API endpoints and types for /v4/nodebalancers upcoming: [M3-9421, M3-9422] - Add & Update existing API endpoints and types for /v4/nodebalancers Mar 12, 2025
@harsh-akamai harsh-akamai changed the title upcoming: [M3-9421, M3-9422] - Add & Update existing API endpoints and types for /v4/nodebalancers upcoming: [ M3-9422] -Update existing API endpoints and types for /v4/nodebalancers Mar 12, 2025
@harsh-akamai harsh-akamai changed the title upcoming: [ M3-9422] -Update existing API endpoints and types for /v4/nodebalancers upcoming: [M3-9422] -Update existing API endpoints and types for /v4/nodebalancers Mar 12, 2025
Copy link

github-actions bot commented Mar 12, 2025

Coverage Report:
Base Coverage: 79.85%
Current Coverage: 79.84%

@harsh-akamai
Copy link
Contributor Author

harsh-akamai commented Mar 12, 2025

This PR is technically M3-9421 and M3-9422, right?

This covers only M3-9422. I've only added /v4beta endpoints for all the existing ones

We don't currently have or use the /v4/nodebalancers/{nodeBalancerId}/configs/{configId}/rebuild endpoint, but I think it'd be worth adding the non-beta and beta versions.

I know you mentioned the /v4/vpcs endpoint work was still pending on the back-end -- is that inclusive of the /v4/nodebalancers/{id}/vpcs ones?

So the /rebuild and /nodebalancers/{id}/vpcs endpoints are new(as per the CM codebase) so I've covered it in a different PR (here).

The endpoints under/v4/vpcs are pending. Everything under /v4(beta)/nodebalancers are covered in these 2 PRs
cc: @dwiley-akamai

@dwiley-akamai dwiley-akamai added the NB-VPC Relating to NodeBalancer-VPC integration label Mar 12, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get confirmation on the data shapes? All but CreateNodeBalancerConfigNode are a little unclear to me based on the API spec (ex: it seems like the API spec suggests via a code snippet that CreateNodeBalancerPayload would have a vpcs property that is an array of objects with the shape you have for the vpc property currently). We should ask that they provide clear examples of the data shapes for each case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the changes w.r.t. creating a nodebalancer have been specified under the POST /v4beta/nodebalancers endpoint in the API spec, which I've used to make the chain of changes in CreateNodeBalancerPayload, CreateNodeBalancerConfig and CreateNodeBalancerConfigNode.
Changes for NodeBalancerConfigNode comes from the GET /v4beta/nodebalancers/{nodeBalancerId}/configs/{configId}/nodes endpoint.

I've asked to add the modifications in request/response json to be added. But I think, for the existing endpoints this shouldn't be a blocker

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 539 passing tests on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing539 Passing3 Skipped102m 43s

@@ -232,4 +235,9 @@ export interface CreateNodeBalancerPayload {
configs: CreateNodeBalancerConfig[];
firewall_id?: number;
tags?: string[];
vpc?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This field should be named vpcs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this. Added the change in PR #11832

@harsh-akamai harsh-akamai merged commit 7956b5d into linode:develop Mar 17, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NB-VPC Relating to NodeBalancer-VPC integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants