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

fix yaml archives for conformance #1206

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
2 changes: 0 additions & 2 deletions cmd/registry/cmd/apply/apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,6 @@ func TestArtifactStorage(t *testing.T) {
}

want := &apihub.Lifecycle{
Id: "lifecycle", // deprecated field
Kind: "Lifecycle", // deprecated field
DisplayName: "Lifecycle",
Description: "A series of stages that an API typically moves through in its lifetime",
Stages: []*apihub.Lifecycle_Stage{
Expand Down
7 changes: 6 additions & 1 deletion cmd/registry/cmd/compute/conformance/conformance.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"github.com/spf13/cobra"
)

var styleguideFilter = fmt.Sprintf("mime_type.contains('%s')", mime.MimeTypeForKind("StyleGuide"))
var styleguideFilter = fmt.Sprintf("mime_type.contains('%s') || mime_type.contains('%s')", mime.MimeTypeForKind("StyleGuide"), mime.YamlMimeTypeForKind("StyleGuide"))

func Command() *cobra.Command {
var filter string
Expand Down Expand Up @@ -82,6 +82,11 @@
log.FromContext(ctx).WithError(err).Debugf("Unmarshal() to StyleGuide failed on artifact: %s", artifact.GetName())
return nil
}
name, err := names.ParseArtifact(artifact.GetName())
if err != nil {
return err
}

Check warning on line 88 in cmd/registry/cmd/compute/conformance/conformance.go

View check run for this annotation

Codecov / codecov/patch

cmd/registry/cmd/compute/conformance/conformance.go#L87-L88

Added lines #L87 - L88 were not covered by tests
guide.Id = name.ArtifactID()
guides = append(guides, guide)
return nil
}); err != nil {
Expand Down
82 changes: 17 additions & 65 deletions cmd/registry/patch/artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -115,54 +113,29 @@
}

func applyArtifactPatch(ctx context.Context, client connection.RegistryClient, content *encoding.Artifact, parent string, filename string) error {
// Restyle the YAML representation so that yaml.Marshal will marshal it as JSON.
encoding.StyleForJSON(&content.Data)
// Marshal the YAML representation into the JSON serialization.
j, err := yaml.Marshal(content.Data)
if err != nil {
return err
}
// Populate Id and Kind fields in the contents of the artifact
jWithIdAndKind, err := populateIdAndKind(j, content.Kind, content.Metadata.Name)
if err != nil {
return err
}
var mimeType string
var bytes []byte
// Unmarshal the JSON serialization into the message struct.
var m proto.Message
m, err = mime.MessageForKind(content.Kind)
if err == nil {
err = protojson.Unmarshal(jWithIdAndKind, m)
msg, err := mime.MessageForKind(content.Kind)
if storeArchivesAsYaml(ctx) || err != nil {
mimeType = mime.YamlMimeTypeForKind(content.Kind)
encoding.StyleForYAML(&content.Data)
bytes, err = yaml.Marshal(content.Data)
if err != nil {
if strings.Contains(err.Error(), "unknown field") {
// Try unmarshaling the original YAML (without the additional Id and Kind fields).
err = protojson.Unmarshal(j, m)
if err != nil {
return err
}
}
}
if storeArchivesAsYaml(ctx) {
mimeType = mime.YamlMimeTypeForKind(content.Kind)
encoding.StyleForYAML(&content.Data)
bytes, err = yaml.Marshal(content.Data)
if err != nil {
return err
}
} else {
mimeType = mime.MimeTypeForKind(content.Kind)
// Marshal the message struct to bytes.
bytes, err = proto.Marshal(m)
if err != nil {
return err
}
return err

Check warning on line 124 in cmd/registry/patch/artifact.go

View check run for this annotation

Codecov / codecov/patch

cmd/registry/patch/artifact.go#L124

Added line #L124 was not covered by tests
}
} else {
// If there was no struct defined for the type, marshal it struct as YAML
// Convert YAML to JSON for protojson
encoding.StyleForJSON(&content.Data)
j, err := yaml.Marshal(content.Data)
if err != nil {
return err
}

Check warning on line 132 in cmd/registry/patch/artifact.go

View check run for this annotation

Codecov / codecov/patch

cmd/registry/patch/artifact.go#L131-L132

Added lines #L131 - L132 were not covered by tests
err = protojson.Unmarshal(j, msg)
if err != nil {
return err
}
mimeType = mime.MimeTypeForKind(content.Kind)
encoding.StyleForYAML(&content.Data)
bytes, err = yaml.Marshal(content.Data)
bytes, err = proto.Marshal(msg)
if err != nil {
return err
}
Expand Down Expand Up @@ -196,27 +169,6 @@
return nil
}

// populateIdAndKind inserts the "id" and "kind" fields in the supplied json bytes.
func populateIdAndKind(bytes []byte, kind, id string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break API hub, which currently reads proto-serialized artifacts and expects them to contain "kind" and "id" fields. Since these fields are redundant in Registry YAML, I don't think we should be including them in YAML-serialized artifacts and I expect that API hub will adapt to that when it starts reading those YAML-serialized artifacts.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's unfortunate. So I need to add this back for protos?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this PR isn't critical until we go all in on yaml...perhaps we should hold it until we correct API Hub.

var jsonData map[string]interface{}
err := json.Unmarshal(bytes, &jsonData)
if err != nil {
return nil, err
}
if jsonData == nil {
return nil, errors.New("missing data")
}
jsonData["id"] = id
jsonData["kind"] = kind

rBytes, err := json.Marshal(jsonData)
if err != nil {
return nil, err
}

return rBytes, nil
}

func UnmarshalContents(contents []byte, mimeType string, message proto.Message) error {
if !mime.IsYamlKind(mimeType) {
return proto.Unmarshal(contents, message)
Expand Down
3 changes: 0 additions & 3 deletions cmd/registry/patch/patch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1185,9 +1185,6 @@ func TestInvalidArtifactPatches(t *testing.T) {
{
artifactID: "references-no-data",
},
{
artifactID: "struct-invalid-structure",
},
{
artifactID: "struct-no-metadata",
},
Expand Down

This file was deleted.