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

Add support of Flavors and FlavorProfiles for Octavia #2575

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Add support of Flavors and FlavorProfiles for Octavia #2575

merged 2 commits into from
Jan 24, 2024

Conversation

Pyjou
Copy link
Contributor

@Pyjou Pyjou commented Feb 28, 2023

Add support of Flavors and FlavorProfiles for Octavia

Fixes #2574

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

https://github.com/openstack/octavia/blob/master/octavia/api/v2/controllers/flavors.py
https://github.com/openstack/octavia/blob/master/octavia/api/v2/controllers/flavor_profiles.py

github-actions[bot]
github-actions bot reviewed Feb 28, 2023
View reviewed changes
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.

@coveralls
Copy link

coveralls commented Feb 28, 2023 •
edited

Coverage Status

coverage: 77.904% (+0.07%) from 77.835%
when pulling 34a7c13 on Pyjou:add-flavor-octavia
into 8587253 on gophercloud:master.

@EmilienM
Copy link
Contributor

EmilienM commented Mar 1, 2023

@Pyjou I had a brief look at this PR and it looks good to me but it'll be nice to test it with acceptance. Let us know if you need help.
Thanks

@Pyjou Pyjou force-pushed the add-flavor-octavia branch 2 times, most recently from 0291578 to e2b8980 Compare March 2, 2023 02:23
@Pyjou Pyjou force-pushed the add-flavor-octavia branch 2 times, most recently from dcb63f0 to cce483b Compare March 2, 2023 03:54
@Pyjou
Copy link
Contributor Author

Pyjou commented Mar 2, 2023

@EmilienM I took inspiration from the other tests and added them.

@Pyjou
Copy link
Contributor Author

Pyjou commented Mar 20, 2023

Any news?

@nikParasyr nikParasyr mentioned this pull request Mar 23, 2023
Closed
@UmBsublime
Copy link

Any chance this could be merged in the next release? This patch is required in order to implement the change also in the openstack terraform provider.
If anything is missing please advise and we will quickly work on required modification.

@mandre
Copy link
Contributor

mandre commented Apr 20, 2023

Hi @Pyjou, sorry for the delay. I'm trying to take a look either today or tomorrow.

mandre
mandre reviewed Apr 21, 2023
View reviewed changes
openstack/loadbalancer/v2/flavors/testing/fixutres.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavors/requests.go Show resolved Hide resolved
openstack/loadbalancer/v2/flavors/requests.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavors/requests.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavorprofiles/requests.go Outdated
}

type ListOpts struct {
ID string `q:"id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as for the Flavors, we should have fields here.

openstack/loadbalancer/v2/flavorprofiles/requests.go Outdated Show resolved Hide resolved
acceptance/openstack/loadbalancer/v2/flavorprofiles_test.go Outdated Show resolved Hide resolved
acceptance/openstack/loadbalancer/v2/flavors_test.go Outdated Show resolved Hide resolved
@mandre
Copy link
Contributor

mandre commented Apr 21, 2023

Thanks a lot for the PR @Pyjou, I can see it was a substantial amount of work. Sorry it took so long for us to take a first pass at it. As a tip for next time, I suggest you break down your PRs into smaller chunks (ideally 1 PR for each API call), so that it's easier for us to review and we can iterate more quickly when there are things to address. This also ensures that we get acceptance tests for all new API calls we add support for.

@EmilienM
Copy link
Contributor

@Pyjou I'm just checking in, are you still looking at this PR? A few comments were left and I want to make sure you're not stuck or anything. Thanks for your contribution!

@github-actions github-actions bot added the semver:minor Backwards-compatible change label Sep 11, 2023
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Sep 11, 2023
@Pyjou
Copy link
Contributor Author

Pyjou commented Sep 15, 2023

@EmilienM and @mandre I pushed a new version of my code. I added a few functional tests.

@yomovh
Copy link

yomovh commented Nov 7, 2023 •
edited

@EmilienM @mandre : is there any thing we can do to make this PR move forward ?
Thanks

@EmilienM
Copy link
Contributor

Hi @Pyjou could you please rebase and once CI is green I'll have a look. Thanks

@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Nov 15, 2023
@pierreyvesv pierreyvesv mentioned this pull request Dec 15, 2023
Closed
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Jan 16, 2024
@Pyjou
Copy link
Contributor Author

Pyjou commented Jan 16, 2024

I fixed tests, wrong import and syntax error.
Can you review it? Thank you

@mandre
Copy link
Contributor

mandre commented Jan 16, 2024

I fixed tests, wrong import and syntax error. Can you review it? Thank you

I'll try my best to review the PR this week.

mandre
mandre reviewed Jan 18, 2024
View reviewed changes
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.

A lot of small things which should be easy to fix, but nothing really dramatic except maybe for the missing omitempty in the flavor update request. It would be nice also to document all the fields. They will end up in the documentation, for example https://pkg.go.dev/github.com/gophercloud/gophercloud@v1.8.0/openstack/loadbalancer/v2/loadbalancers#CreateOpts

Otherwise, really good work. This is close to being mergeable.

internal/acceptance/openstack/loadbalancer/v2/loadbalancer.go Outdated Show resolved Hide resolved
internal/acceptance/openstack/loadbalancer/v2/loadbalancer.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavorprofiles/doc.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavorprofiles/doc.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavorprofiles/doc.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavors/requests.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavorprofiles/requests.go Show resolved Hide resolved
openstack/loadbalancer/v2/flavorprofiles/requests.go Show resolved Hide resolved
openstack/loadbalancer/v2/flavors/requests.go Outdated Show resolved Hide resolved
openstack/loadbalancer/v2/flavors/requests.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Jan 23, 2024
@github-actions github-actions bot added semver:minor Backwards-compatible change and removed semver:minor Backwards-compatible change labels Jan 24, 2024
@Pyjou
Copy link
Contributor Author

Pyjou commented Jan 24, 2024

A lot of small things which should be easy to fix, but nothing really dramatic except maybe for the missing omitempty in the flavor update request. It would be nice also to document all the fields. They will end up in the documentation, for example https://pkg.go.dev/github.com/gophercloud/gophercloud@v1.8.0/openstack/loadbalancer/v2/loadbalancers#CreateOpts

Otherwise, really good work. This is close to being mergeable.

You can review now :)
As requested, I've added comments to the code.

mandre
mandre reviewed Jan 24, 2024
View reviewed changes
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.

LGTM, let's see if the tests agree.

@mandre
Copy link
Contributor

mandre commented Jan 24, 2024

For the jobs that failed, it timed out while creating the LB. Retrying.

@mandre mandre merged commit b7f2d15 into gophercloud:master Jan 24, 2024
28 checks passed
@EmilienM EmilienM added the backport-v1 This PR will be backported to v1 label Feb 2, 2024
@gophercloud-backport-bot gophercloud-backport-bot bot mentioned this pull request Feb 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@mandre mandre mandre approved these changes

Assignees
No one assigned
Labels
backport-v1 This PR will be backported to v1 semver:minor Backwards-compatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support of Flavors and FlavorProfiles for Octavia
6 participants
@Pyjou @coveralls @EmilienM @UmBsublime @mandre @yomovh