feat(dqlite): add service layer for cloud image metadata #18121

gfouillet · 2024-09-19T12:23:48Z

Add the service layer for cloud image metadata.

It is basically a wrapper to state, with few functionnalities:

Validation of metadata (missing required attributes, empty image ID or invalid architectures)
Groups metadata by source while fetching (to be as close as possible to actual interface)

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
~~- [ ] Integration tests, with comments saying what you're testing~~
~~- [ ] doc.go added or updated in changed packages~~

QA steps

Unit test are self-sufficient

Documentation changes

None

Links

Jira card: JUJU-6630

nvinuesa

First pass, looking very good. Just a few small remarks/questions.

core/cloudimagemetadata/types.go

nvinuesa · 2024-09-19T14:41:22Z

domain/cloudimagemetadata/service/service.go

+// invalid.
+func (s Service) SaveMetadata(ctx context.Context, metadata []cloudimagemetadata.Metadata) error {
+	if len(metadata) == 0 {
+		return errors.NothingToSave


Why this error? We don't return it on the legacy state, and it does feel a bit counter-intuitive.

Yes you are right, it is not on the legacy. I think somewhere in my mind I think that caller shouldn't even try to save empty metadata... But there would be inconsistent of some generic function in go behave (like errors.Join).

Will return nil.

nvinuesa · 2024-09-19T14:56:14Z

domain/cloudimagemetadata/service/service.go

+
+	supportedArchitectures := s.st.SupportedArchitectures(context.Background())
+	if !supportedArchitectures.Contains(m.Arch) {
+		return jujuerrors.Errorf("unsupported architecture %s (should be any of %s): %w", m.Arch, supportedArchitectures.Values(), errors.Invalid)


Just to be sure, errors.NotValid(..) returns an error that satisfies errors.Is(err, errors.Invalid) right? If not, why not using errors.NotValidf(...?

It returns an error who satisfies github.com/juju/juju/domain/cloudimagemetadata/errors.Invalid.

I don't know from which errors package you get error.NotValidf but I assumes it comes from github.com/juju/juju/errors which we don't use anymore, as far i understand, in favor of domain-scoped errors.

Oh I see, it's from your domain errors' package. That's fine then. IMO you should name the error NotValid as well, just for consistency because that's what we have all around our codebase.

core/cloudimagemetadata/types.go

domain/cloudimagemetadata/service/service.go

nvinuesa

LGTM, thanks for the changes!

nvinuesa · 2024-09-20T12:31:26Z

domain/cloudimagemetadata/errors/errors.go

 	// fields empty.
-	Invalid = errors.ConstError("invalid metadata")
+	NotValid = errors.ConstError("invalid metadata")


SimonRichardson

Fix this context issue, then this is good to go.

SimonRichardson · 2024-09-20T13:53:16Z

domain/cloudimagemetadata/service/service.go

+	if imageId == "" {
+		return errors.EmptyImageID
+	}


domain/cloudimagemetadata/service/service.go

SimonRichardson

Thanks for guiding this through.

gfouillet · 2024-09-20T14:54:54Z

/merge

hmlanigan · 2024-09-20T16:34:20Z

domain/cloudimagemetadata/service/package_test.go

@@ -0,0 +1,16 @@
+// Copyright 2023 Canonical Ltd.


todo: s/2023/2024

hmlanigan · 2024-09-20T16:35:41Z

domain/cloudimagemetadata/service/service.go

+
+	// FindMetadata retrieves a list of cloud image metadata based on the provided criteria.
+	// It returns the matched metadata or a [github.com/juju/juju/domain/cloudimagemetadata/errors.NotFound] error
+	// If there are no one.


nit:

Suggested change

// If there are no one.

// if there are not one.

hmlanigan · 2024-09-20T16:38:10Z

domain/cloudimagemetadata/service/service.go

+}
+
+// SaveMetadata saves the provided cloud image metadata if non-empty and valid.
+// It returns a [errors.NothingToSave] if the input are empty, or a [errors.NotValid] if at least one of the inputs are


question: where is error NothingToSave defined? Looks like the method returns nil in this case, which is preferable.

It is not. Removed it during review, forgot to remove this part of comment. Thanks for asking, I removed it.

hmlanigan · 2024-09-20T16:38:43Z

domain/cloudimagemetadata/service/service.go

+	return s.st.SaveMetadata(ctx, metadata)
+}
+
+// DeleteMetadataWithImageID removes all the metadata associated with the given imageID from the state.


todo: add errors returned to the comments.

hmlanigan · 2024-09-20T16:40:00Z

domain/cloudimagemetadata/service/service.go

+
+	var missing []string
+	if m.Version == "" {
+		missing = append(missing, "version")


love it - gathering all the errors before returning.

Signed-off-by: Guillaume Fouillet <guillaume.fouillet@canonical.com>

- Before this commit, the code lacked a service layer interface for managing cloud image metadata. - After this commit, a service interface for cloud image metadata management is added along with its unit tests. This commit introduces a new service layer for managing cloud image metadata, including methods for saving, deleting, and finding metadata. Signed-off-by: Guillaume Fouillet <guillaume.fouillet@canonical.com>

gfouillet · 2024-09-23T12:17:37Z

/merge

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch 4 times, most recently from f360fbb to a07610b Compare September 19, 2024 14:12

gfouillet requested review from hmlanigan, barrettj12, nvinuesa and SimonRichardson September 19, 2024 14:33

nvinuesa reviewed Sep 19, 2024

View reviewed changes

SimonRichardson reviewed Sep 19, 2024

View reviewed changes

core/cloudimagemetadata/types.go Show resolved Hide resolved

domain/cloudimagemetadata/service/service.go Outdated Show resolved Hide resolved

domain/cloudimagemetadata/service/service.go Outdated Show resolved Hide resolved

domain/cloudimagemetadata/service/service.go Show resolved Hide resolved

hpidcock added the 4.0 label Sep 20, 2024

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch from a07610b to 889aadf Compare September 20, 2024 08:21

gfouillet requested review from SimonRichardson and nvinuesa September 20, 2024 08:21

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch 2 times, most recently from d4e7bfc to da8f8f4 Compare September 20, 2024 12:21

nvinuesa approved these changes Sep 20, 2024

View reviewed changes

SimonRichardson reviewed Sep 20, 2024

View reviewed changes

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch from da8f8f4 to 513a5e6 Compare September 20, 2024 14:03

SimonRichardson approved these changes Sep 20, 2024

View reviewed changes

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch from 513a5e6 to 2c14502 Compare September 20, 2024 15:15

hmlanigan reviewed Sep 20, 2024

View reviewed changes

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch from 2c14502 to 807d1c5 Compare September 23, 2024 07:40

gfouillet added 2 commits September 23, 2024 13:30

refactor(dqlite): rename Metadata.ImageId to Metadata.ImageID

3737440

Signed-off-by: Guillaume Fouillet <guillaume.fouillet@canonical.com>

gfouillet force-pushed the v4/dqlite/cloudimagemetadata/service branch from 807d1c5 to bc2521b Compare September 23, 2024 11:31

jujubot merged commit a5c1e1b into juju:main Sep 23, 2024
19 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dqlite): add service layer for cloud image metadata #18121

feat(dqlite): add service layer for cloud image metadata #18121

gfouillet commented Sep 19, 2024 •

edited by jira bot

Loading

nvinuesa left a comment

nvinuesa Sep 19, 2024

gfouillet Sep 19, 2024

nvinuesa Sep 19, 2024

gfouillet Sep 19, 2024

nvinuesa Sep 20, 2024

nvinuesa left a comment

nvinuesa Sep 20, 2024

SimonRichardson left a comment

SimonRichardson Sep 20, 2024

SimonRichardson left a comment

gfouillet commented Sep 20, 2024

hmlanigan Sep 20, 2024

hmlanigan Sep 20, 2024

hmlanigan Sep 20, 2024

gfouillet Sep 23, 2024

hmlanigan Sep 20, 2024

hmlanigan Sep 20, 2024

gfouillet commented Sep 23, 2024

feat(dqlite): add service layer for cloud image metadata #18121

feat(dqlite): add service layer for cloud image metadata #18121

Conversation

gfouillet commented Sep 19, 2024 • edited by jira bot Loading

Checklist

QA steps

Documentation changes

Links

nvinuesa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvinuesa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson left a comment

Choose a reason for hiding this comment

gfouillet commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfouillet commented Sep 23, 2024

gfouillet commented Sep 19, 2024 •

edited by jira bot

Loading