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 octavia pool member batch update BatchUpdateMembersAdditiveOnly #2863

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdrienPensart
Copy link

@github-actions github-actions bot added the semver:minor Backwards-compatible change label Jan 11, 2024
@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from 73c3358 to b90b180 Compare January 11, 2024 17:10
@github-actions github-actions bot removed the semver:minor Backwards-compatible change label Jan 11, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.

@github-actions github-actions bot added the semver:minor Backwards-compatible change label Jan 11, 2024
@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from b90b180 to 668c2aa Compare January 11, 2024 17:11
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Jan 11, 2024
Comment on lines 499 to 509
members := []map[string]interface{}{}
for _, opt := range opts {
b, err := opt.ToBatchMemberUpdateMap()
if err != nil {
r.Err = err
return
}
members = append(members, b)
}

b := map[string]interface{}{"members": members}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like code duplication with BatchUpdateMembers() that you could minimize by extracting this into a separate function.

@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from 668c2aa to 6f1ff3f Compare January 11, 2024 18:54
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Jan 11, 2024
Copy link
Contributor

@mandre mandre left a comment

Choose a reason for hiding this comment

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

The code looks good, but I'd like to see acceptance tests showing that the new BatchUpdateMembersAdditiveOnly() function does what is claims to do. Can we add that?

@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from 6f1ff3f to 38a163e Compare February 9, 2024 15:28
@github-actions github-actions bot added semver:unknown Unable to figure out the semver type and removed semver:minor Backwards-compatible change labels Feb 9, 2024
@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from 38a163e to 8ff2c2d Compare February 9, 2024 19:22
@github-actions github-actions bot added semver:major Breaking change and removed semver:unknown Unable to figure out the semver type labels Feb 9, 2024
@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from 8ff2c2d to f7e3160 Compare April 10, 2024 14:48
@github-actions github-actions bot removed the semver:major Breaking change label Apr 10, 2024
Signed-off-by: Adrien Pensart <crunchengine@gmail.com>
@AdrienPensart AdrienPensart force-pushed the octavia_pool_member_batch_update_additive_only branch from f7e3160 to 6d842c6 Compare April 10, 2024 14:49
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Apr 10, 2024
@AdrienPensart
Copy link
Author

acceptance tests added!

@AdrienPensart
Copy link
Author

Can we have a review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor Backwards-compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants