Skip to content

Add CRUD support for register limit APIs#2616

Merged
mandre merged 1 commit intogophercloud:masterfrom
konan-abhi:registered_limits
May 23, 2023
Merged

Add CRUD support for register limit APIs#2616
mandre merged 1 commit intogophercloud:masterfrom
konan-abhi:registered_limits

Conversation

@konan-abhi
Copy link
Copy Markdown
Contributor

@konan-abhi konan-abhi commented May 10, 2023 •
edited
Loading

github-actions[bot]
github-actions bot reviewed May 10, 2023
View reviewed changes
Copy link
Copy Markdown

@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.

@fmount fmount mentioned this pull request May 10, 2023
Closed
fmount added a commit to fmount/glance-operator that referenced this pull request May 10, 2023
@fmount
Enforce Per-Tenant Quotas configuration
fd5f080 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
@konan-abhi konan-abhi force-pushed the registered_limits branch 2 times, most recently from 4b90541 to 59f52e1 Compare May 12, 2023 04:25
@konan-abhi konan-abhi changed the title WIP - Add support for registering default limit across projects Add CRUD support for register limit APIs May 12, 2023
@konan-abhi konan-abhi force-pushed the registered_limits branch 7 times, most recently from bdaf4d5 to 9412d59 Compare May 15, 2023 08:12
@fmount fmount mentioned this pull request May 15, 2023
Merged
@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 16, 2023

@konan-abhi is this ready for review? Otherwise, I suggest you mark your PR as draft while you're still working on it.

@konan-abhi
Copy link
Copy Markdown
Contributor Author

konan-abhi commented May 16, 2023

@konan-abhi is this ready for review? Otherwise, I suggest you mark your PR as draft while you're still working on it.

@konan-abhi is this ready for review? Otherwise, I suggest you mark your PR as draft while you're still working on it.

Hi @mandre I am struggling to fix unit tests (since not able to run/debug them locally). Any inputs there will be helpful. Other than that I think functional and other tests are working as expected and yes apart from unittests failure the patch is ready for review.

mandre
mandre reviewed May 16, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Hi @mandre I am struggling to fix unit tests (since not able to run/debug them locally). Any inputs there will be helpful. Other than that I think functional and other tests are working as expected and yes apart from unittests failure the patch is ready for review.

Contrary to acceptance tests, you don't need a live environment to run the unit tests. You can run them locally with:

go test -v ./openstack/identity/v3/registered_limits/testing

That should make it easier to fix the unit tests.

I've pointed to the errors that are currently reported, but there a few more.

@nikParasyr
Copy link
Copy Markdown
Contributor

nikParasyr commented May 16, 2023

This should close #2290, or partially implement. Just linking the PR with the issue

@konan-abhi konan-abhi force-pushed the registered_limits branch from 9412d59 to 5101958 Compare May 16, 2023 14:37
@konan-abhi
Copy link
Copy Markdown
Contributor Author

konan-abhi commented May 16, 2023

Hi @mandre I am struggling to fix unit tests (since not able to run/debug them locally). Any inputs there will be helpful. Other than that I think functional and other tests are working as expected and yes apart from unittests failure the patch is ready for review.

Contrary to acceptance tests, you don't need a live environment to run the unit tests. You can run them locally with:

go test -v ./openstack/identity/v3/registered_limits/testing

That should make it easier to fix the unit tests.

I've pointed to the errors that are currently reported, but there a few more.

Thank you,
I am able to run tests locally and passing as well.

@konan-abhi konan-abhi force-pushed the registered_limits branch from 5101958 to 6172e89 Compare May 16, 2023 14:44
@konan-abhi
Copy link
Copy Markdown
Contributor Author

konan-abhi commented May 16, 2023

Does the failure in unit test means missing acceptance test for Get call?

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 16, 2023

Does the failure in unit test means missing acceptance test for Get call?

It complains that the comment doesn't follow go formatting.
You need to run go fmt on openstack/identity/v3/registered_limits/doc.go, using go 1.19+.

@konan-abhi konan-abhi force-pushed the registered_limits branch 2 times, most recently from 8413e39 to 936dcb9 Compare May 16, 2023 16:12
konan-abhi added a commit to konan-abhi/lib-common that referenced this pull request May 16, 2023
@konan-abhi
Add limits module
ceca63f 
E.g. used in keystone service to register limits for particular
service.

Depends-On: gophercloud/gophercloud/pull/2616
@konan-abhi konan-abhi mentioned this pull request May 16, 2023
Merged
konan-abhi added a commit to konan-abhi/lib-common that referenced this pull request May 17, 2023
@konan-abhi
Add limits module
740f94e 
E.g. used in keystone service to register limits for particular
service.

Depends-On: gophercloud/gophercloud/pull/2616
@fmount
Copy link
Copy Markdown

fmount commented May 17, 2023 •
edited
Loading

Does the failure in unit test means missing acceptance test for Get call?

It complains that the comment doesn't follow go formatting. You need to run go fmt on openstack/identity/v3/registered_limits/doc.go, using go 1.19+.

I also see the unit tests failing:

Error: /home/runner/go/pkg/mod/golang.org/x/tools@v0.9.1/internal/gocommand/invoke.go:381:62: undefined: os.ErrProcessDone
note: module requires Go 1.18
Error: Process completed with exit code 2.

@mandre any particular reason to stay on 1.14 ?
We also saw failures if we try to bump something in go.mod (if [ $(go mod tidy && git diff | wc -l) -gt 0 ]; then git diff && exit 1; fi), so I @konan-abhi I suspect you should restore go.{mod, sum} locally and try to run the unit tests

@fmount
Copy link
Copy Markdown

fmount commented May 17, 2023

In general the patch passes locally:

> go test -v ./openstack/identity/v3/registered_limits/testing
=== RUN   TestListRegisteredLimits
--- PASS: TestListRegisteredLimits (0.00s)
=== RUN   TestListRegisteredLimitsAllPages
--- PASS: TestListRegisteredLimitsAllPages (0.00s)
=== RUN   TestCreateRegisteredLimits
--- PASS: TestCreateRegisteredLimits (0.00s)
=== RUN   TestGetRegisteredLimit
--- PASS: TestGetRegisteredLimit (0.00s)
=== RUN   TestDeleteRegisteredLimit
--- PASS: TestDeleteRegisteredLimit (0.00s)
=== RUN   TestUpdateRegisteredLimit
--- PASS: TestUpdateRegisteredLimit (0.00s)
PASS
ok      github.com/gophercloud/gophercloud/openstack/identity/v3/registered_limits/testing      0.005s

I suspect we should address something related to the CI (functional-identity tests instead are failing for unrelated issues)

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 22, 2023

The acceptance tests didn't run because we're missing the OS_BRANCH variable in the job definition. I'm fixing this with #2619.

@konan-abhi konan-abhi force-pushed the registered_limits branch 2 times, most recently from c1354cb to cc2f38f Compare May 22, 2023 07:59
mandre
mandre reviewed May 22, 2023
View reviewed changes
acceptance/openstack/identity/v3/registeredlimits_test.go Outdated
defer os.Unsetenv("OS_SYSTEM_SCOPE")

clients.RequireAdmin(t)
clients.SkipReleasesBelow(t, "stable/xena")
Copy link
Copy Markdown
Contributor

@mandre mandre May 22, 2023

Choose a reason for hiding this comment

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

Unless we're using an API endpoint that is only available from Xena there is no reason to add a skip.

Copy link
Copy Markdown
Contributor Author

@konan-abhi konan-abhi May 22, 2023

Choose a reason for hiding this comment

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

Done

acceptance/openstack/identity/v3/registeredlimits_test.go Outdated
th "github.com/gophercloud/gophercloud/testhelper"
)

func TestRegisteredLimitsList(t *testing.T) {
Copy link
Copy Markdown
Contributor

@mandre mandre May 22, 2023

Choose a reason for hiding this comment

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

We should probably combine the List test within TestRegisteredLimitsCRUD so that we have openstack return results.

Copy link
Copy Markdown
Contributor Author

@konan-abhi konan-abhi May 22, 2023

Choose a reason for hiding this comment

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

Done

acceptance/openstack/identity/v3/registeredlimits_test.go Outdated
th.AssertNoErr(t, err)
}

func TestCreateRegisteredLimits(t *testing.T) {
Copy link
Copy Markdown
Contributor

@mandre mandre May 22, 2023

Choose a reason for hiding this comment

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

Same, this could be part of TestRegisteredLimitsCRUD().

Copy link
Copy Markdown
Contributor Author

@konan-abhi konan-abhi May 22, 2023

Choose a reason for hiding this comment

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

Done

acceptance/openstack/identity/v3/registeredlimits_test.go Outdated
client, err := clients.NewIdentityV3Client()
th.AssertNoErr(t, err)

// Find image service (glance on Devstack)
Copy link
Copy Markdown
Contributor

@mandre mandre May 22, 2023

Choose a reason for hiding this comment

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

We could just pick the first service (doesn't have to be glance) to simplify the test a little.

Copy link
Copy Markdown
Contributor Author

@konan-abhi konan-abhi May 22, 2023

Choose a reason for hiding this comment

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

Done

acceptance/openstack/identity/v3/registeredlimits_test.go Outdated
th.AssertEquals(t, resourceName, createdRegisteredLimits[0].ResourceName)
th.AssertEquals(t, serviceID, createdRegisteredLimits[0].ServiceID)

//Delete the registered limit
Copy link
Copy Markdown
Contributor

@mandre mandre May 22, 2023

Choose a reason for hiding this comment

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

Nit: as a general formatting rule, try leaving a space before the comment.

Copy link
Copy Markdown
Contributor Author

@konan-abhi konan-abhi May 22, 2023

Choose a reason for hiding this comment

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

Done

openstack/identity/v3/registeredlimits/requests.go Outdated
ResourceName string `json:"resource_name" required:"true"`

// DefaultLimit is the default limit.
DefaultLimit int `json:"default_limit"`
Copy link
Copy Markdown
Contributor

@mandre mandre May 22, 2023

Choose a reason for hiding this comment

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

default_limit is also a required parameter according to the doc.

Copy link
Copy Markdown
Contributor Author

@konan-abhi konan-abhi May 22, 2023

Choose a reason for hiding this comment

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

Done

@konan-abhi konan-abhi force-pushed the registered_limits branch from cc2f38f to 23b9b15 Compare May 22, 2023 10:34
@konan-abhi
Copy link
Copy Markdown
Contributor Author

konan-abhi commented May 23, 2023

Ready for review!

konan-abhi added a commit to konan-abhi/lib-common that referenced this pull request May 23, 2023
@konan-abhi
Add limits module
14a3474 
E.g. used in keystone service to register limits for particular
service.

Depends-On: gophercloud/gophercloud/pull/2616
@mandre mandre added this to the v1.4.0 milestone May 23, 2023
@mandre mandre merged commit c8549f4 into gophercloud:master May 23, 2023
@konan-abhi
Copy link
Copy Markdown
Contributor Author

konan-abhi commented May 23, 2023

@mandre Thank you for review, may I know when we can expect the release of 1.4.0? Our new development work depends on this new release.

Thank you!

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 23, 2023

@mandre Thank you for review, may I know when we can expect the release of 1.4.0? Our new development work depends on this new release.

We don't know yet when we'll release 1.4.0, probably in not too long.

In the meantime, you may want to depend on the master branch, or on a fork that you control.

@fmount
Copy link
Copy Markdown

fmount commented May 23, 2023

@mandre Thank you for review, may I know when we can expect the release of 1.4.0? Our new development work depends on this new release.

We don't know yet when we'll release 1.4.0, probably in not too long.

In the meantime, you may want to depend on the master branch, or on a fork that you control.

Thanks @mandre for the input, do you have general timeline that is usually followed as a general rule when bumping from a tag to the next one?

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 25, 2023

Thanks @mandre for the input, do you have general timeline that is usually followed as a general rule when bumping from a tag to the next one?

There's no strict rule, it generally depends on whether there's activity in the open PRs, and how long since the last tag. We try to release every other month.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented May 25, 2023

@konan-abhi @fmount FYI, there's now a v1.4.0 version that includes your change.

@konan-abhi
Copy link
Copy Markdown
Contributor Author

konan-abhi commented May 25, 2023

Thank you very much!!

@nikParasyr nikParasyr mentioned this pull request May 26, 2023
Closed
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 6, 2023
@fmount
Enforce Per-Tenant Quotas configuration
3dc2378 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 6, 2023
@fmount
Enforce Per-Tenant Quotas configuration
ca6f987 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 10, 2023
@fmount
Enforce Per-Tenant Quotas configuration
3101679 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 10, 2023
@fmount
Enforce Per-Tenant Quotas configuration
d37d117 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 10, 2023
@fmount
Enforce Per-Tenant Quotas configuration
ca6cc0a 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 10, 2023
@fmount
Enforce Per-Tenant Quotas configuration
f8ba8a3 
This patch exposes a boolean that can be used to enforce per-tenant
quotas in the GlanceAPI.
If true, the related glance-api section is enabled and templateParameters
are populated.
This patch assumes the quota resources are already registered in
keystone.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616
Depends-On: openstack-k8s-operators/lib-common/pull/284

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 10, 2023
@fmount
Enforce Per-Tenant Quotas configuration
f3a1b21 
This patch exposes the data structure and implement the related logic
that can be used to enforce per-tenant quotas in the GlanceAPI. When
limits are defined, the related glance-api config section is enabled and
templateParameters are populated.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616
Depends-On: openstack-k8s-operators/lib-common/pull/284

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 12, 2023
@fmount
Enforce Per-Tenant Quotas configuration
5c1ae9b 
This patch exposes the data structure and implement the related logic
that can be used to enforce per-tenant quotas in the GlanceAPI. When
limits are defined, the related glance-api config section is enabled and
templateParameters are populated.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616
Depends-On: openstack-k8s-operators/lib-common/pull/284

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 12, 2023
@fmount
Enforce Per-Tenant Quotas configuration
97f9012 
This patch exposes the data structure and implement the related logic
that can be used to enforce per-tenant quotas in the GlanceAPI. When
limits are defined, the related glance-api config section is enabled and
templateParameters are populated.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616
Depends-On: openstack-k8s-operators/lib-common/pull/284

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 12, 2023
@fmount
Enforce Per-Tenant Quotas configuration
5aae648 
This patch exposes the data structure and implement the related logic
that can be used to enforce per-tenant quotas in the GlanceAPI. When
limits are defined, the related glance-api config section is enabled and
templateParameters are populated.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616
Depends-On: openstack-k8s-operators/lib-common/pull/284

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
fmount added a commit to fmount/glance-operator that referenced this pull request Jun 12, 2023
@fmount
Enforce Per-Tenant Quotas configuration
460f094 
This patch exposes the data structure and implement the related logic
that can be used to enforce per-tenant quotas in the GlanceAPI. When
limits are defined, the related glance-api config section is enabled and
templateParameters are populated.

Depends-On: openstack-k8s-operators/lib-common/pull/262
Depends-On: gophercloud/gophercloud/pull/2616
Depends-On: openstack-k8s-operators/lib-common/pull/284

Signed-off-by: Francesco Pantano <fpantano@redhat.com>
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[bot] left review comments

@mandre mandre mandre approved these changes

+1 more reviewer

@nikParasyr nikParasyr nikParasyr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@konan-abhi @mandre @nikParasyr @fmount @coveralls