Manila: add Get for share-access-rules API by gman0 · Pull Request #2496 · gophercloud/gophercloud

gman0 · 2022-10-20T12:58:52Z

This PR adds Get for share-access-rules Manila API.

Part of #2495

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

https://github.com/openstack/manila/blob/stable/yoga/manila/db/sqlalchemy/api.py#L2591-L2601
https://github.com/openstack/manila/blob/stable/yoga/manila/api/v2/share_accesses.py#L39-L45
https://github.com/openstack/manila/blob/stable/yoga/manila/api/views/share_accesses.py#L50-L62

gman0 · 2022-10-20T13:01:13Z

Ready for reviews. PTAL @mandre @gouthampacha, thanks!

coveralls · 2022-10-20T13:05:37Z

Coverage increased (+0.02%) to 80.07% when pulling 41a6bc5 on gman0:shareaccessrules-get into 92af431 on gophercloud:master.

mandre · 2022-11-09T16:51:25Z

I'll try my best to clear my backlog of reviews before the end of the week, sorry for the time it takes.

gman0 · 2022-11-10T08:46:51Z

Thanks, @mandre!

gman0 · 2022-11-10T08:47:53Z

I've just added an acceptance test but it's failing:

=== RUN   TestShareAccessRulesGet
    shareaccessrules_test.go:29: Unable to grant access to share 70959eca-bcb4-4999-afaf-3e371e044ac1: Resource not found: [POST http://10.1.0.220/share/v2/shares/70959eca-bcb4-4999-afaf-3e371e044ac1/action], error message: {"itemNotFound": {"code": 404, "message": "The resource could not be found."}}
    shares.go:98: Deleted share: 70959eca-bcb4-4999-afaf-3e371e044ac1
--- FAIL: TestShareAccessRulesGet (4.38s)

https://github.com/gophercloud/gophercloud/actions/runs/3429349022/jobs/5714846083#step:5:1097

I'll have a look ASAP, but if anyone has ideas what's wrong please let me know.

mandre · 2022-11-10T10:18:57Z

I've just added an acceptance test but it's failing:
=== RUN   TestShareAccessRulesGet
    shareaccessrules_test.go:29: Unable to grant access to share 70959eca-bcb4-4999-afaf-3e371e044ac1: Resource not found: [POST http://10.1.0.220/share/v2/shares/70959eca-bcb4-4999-afaf-3e371e044ac1/action], error message: {"itemNotFound": {"code": 404, "message": "The resource could not be found."}}
    shares.go:98: Deleted share: 70959eca-bcb4-4999-afaf-3e371e044ac1
--- FAIL: TestShareAccessRulesGet (4.38s)
https://github.com/gophercloud/gophercloud/actions/runs/3429349022/jobs/5714846083#step:5:1097

I'll have a look ASAP, but if anyone has ideas what's wrong please let me know.

I haven't looked into the details but according to #1795 (comment), setting the microversion to 2.49 helps.

mandre · 2022-11-10T10:08:39Z

openstack/sharedfilesystems/v2/shareaccessrules/results.go

+	type tmp ShareAccess
+	var s struct {
+		tmp
+		CreatedAt gophercloud.JSONRFC3339MilliNoZ `json:"created_at"`


According to the doc, the date is returned in ISO 8601 format. Was JSONRFC3339MilliNoZ perhaps an old date format used by Manila?

It seems RFC3339 offers ISO 8601 interoperability: https://www.rfc-editor.org/rfc/rfc3339. Also other structs seem to use this JSONRFC3339MilliNoZ where the docs mention ISO 8601... I'm not an expert in this area

I don't get the question perhaps, all date and timestamp fields from openstack services use this datatype within the gophercloud SDK:

gophercloud/results.go

Line 308 in 5137346

type JSONRFC3339MilliNoZ time.Time

I'm not exactly an expert either in date format either, but it looks like we expect the 2006-01-02T15:04:05.999999 format for JSONRFC3339MilliNoZ, which is different from what Manila claims to return:

The date and time stamp format is ISO 8601:

CCYY-MM-DDThh:mm:ss±hh:mm

The ±hh:mm value, if included, returns the time zone as an offset from UTC.

For example, 2019-03-27T09:49:58-05:00.

Playing a little bit with it, it seems to break as soon as the date include an UTC offset: https://go.dev/play/p/pJh7jLgonx6. It looks like we're just being lucky.

See for example how we parse the dates in neutron: https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/networks/results.go#L101-L131

I've reported #2507 for the date parsing issue if anyone wants to take a crack at it.

gouthampacha · 2022-11-17T23:09:01Z

acceptance/openstack/sharedfilesystems/v2/shareaccessrules_test.go

+		t.Fatalf("Unable to create a shared file system client: %v", err)
+	}
+
+	client.Microversion = "2.49"


this API should be available from "2.45": https://docs.openstack.org/api-ref/shared-file-system/?expanded=list-access-rules-deprecated-detail,list-shares-detail#share-access-rules-since-api-v2-45

Indeed, in general we should try to stick to the minimum needed microversion, although in that particular case, 2.49 is Stein and we're running only newer versions in our CI so that isn't a big deal.

gouthampacha · 2022-11-17T23:12:18Z

Thanks @gman0 - the code LGTM!

mandre

Not necessarily for this PR since the issue seems to be more generalized in the sharedfilesystems module, and possibly elsewhere, but the date parsing seems fragile to me and it would be nice to clean it at some point.

mandre · 2022-11-18T08:51:48Z

openstack/sharedfilesystems/v2/shareaccessrules/results.go

+	type tmp ShareAccess
+	var s struct {
+		tmp
+		CreatedAt gophercloud.JSONRFC3339MilliNoZ `json:"created_at"`


I'm not exactly an expert either in date format either, but it looks like we expect the 2006-01-02T15:04:05.999999 format for JSONRFC3339MilliNoZ, which is different from what Manila claims to return:

The date and time stamp format is ISO 8601:

CCYY-MM-DDThh:mm:ss±hh:mm

The ±hh:mm value, if included, returns the time zone as an offset from UTC.

For example, 2019-03-27T09:49:58-05:00.

Playing a little bit with it, it seems to break as soon as the date include an UTC offset: https://go.dev/play/p/pJh7jLgonx6. It looks like we're just being lucky.

See for example how we parse the dates in neutron: https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/networks/results.go#L101-L131

mandre · 2022-11-18T08:59:08Z

acceptance/openstack/sharedfilesystems/v2/shareaccessrules_test.go

+		t.Fatalf("Unable to create a shared file system client: %v", err)
+	}
+
+	client.Microversion = "2.49"


Indeed, in general we should try to stick to the minimum needed microversion, although in that particular case, 2.49 is Stein and we're running only newer versions in our CI so that isn't a big deal.

gman0 mentioned this pull request Oct 20, 2022

Add new share-access-rules API for Manila #2495

Open

4 tasks

gman0 force-pushed the shareaccessrules-get branch from 6ce5ac4 to 57997d8 Compare November 9, 2022 15:09

mandre reviewed Nov 10, 2022

View reviewed changes

mandre added the semver:minor label Nov 10, 2022

Manila: add Get for share-access-rules API

41a6bc5

gman0 force-pushed the shareaccessrules-get branch from 57997d8 to 41a6bc5 Compare November 17, 2022 17:18

github-actions bot removed the semver:minor label Nov 17, 2022

gouthampacha reviewed Nov 17, 2022

View reviewed changes

mandre added the semver:minor label Nov 18, 2022

mandre reviewed Nov 18, 2022

View reviewed changes

mandre mentioned this pull request Nov 18, 2022

Manila may choke on dates with UTC offset #2507

Open

mandre approved these changes Nov 18, 2022

View reviewed changes

mandre merged commit 3002039 into gophercloud:master Nov 18, 2022

pierreprinetti added this to the v1.1.0 milestone Nov 24, 2022

Conversation

gman0 commented Oct 20, 2022

Uh oh!

gman0 commented Oct 20, 2022

Uh oh!

coveralls commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandre commented Nov 9, 2022

Uh oh!

gman0 commented Nov 10, 2022

Uh oh!

gman0 commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandre commented Nov 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gouthampacha commented Nov 17, 2022

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Oct 20, 2022 •

edited

Loading

gman0 commented Nov 10, 2022 •

edited

Loading