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 object store TOML definitions, add test data #715

Merged
merged 2 commits into from
Nov 16, 2022

Conversation

joeshaw
Copy link
Member

@joeshaw joeshaw commented Nov 15, 2022

This fixes the object store configs to match the Viceroy implementation, and adds object store definitions to the test data.

I think it remains an open question as to whether we should change the Viceroy implementation to be more consistent with the existing local server definitions (backends, dictionaries).

@joeshaw
Copy link
Member Author

joeshaw commented Nov 15, 2022

The inconsistencies I see:

  1. The name of the object store section is object_store, singular. backends and dictionaries are plural.
  2. Dictionaries are populated with a struct containing a key-value map, whereas object stores are populated with a list of structs. Personally I think the object store implementation is nicer to use, but they are different.
  3. For loading data from files, dictionaries uses file and object_store uses path.

Go structure for dictionaries:

type LocalServer struct {
	Backends     map[string]LocalBackend       `toml:"backends"`
	Dictionaries map[string]LocalDictionary    `toml:"dictionaries,omitempty"`
	ObjectStore  map[string][]LocalObjectStore `toml:"object_store,omitempty"`
}

type LocalDictionary struct {
	File     string            `toml:"file,omitempty"`
	Format   string            `toml:"format"`
	Contents map[string]string `toml:"contents,omitempty"`
}

which maps to a toml like:

[local_server]

  [local_server.dictionaries]

    [local_server.dictionaries.toml]
      format = "inline-toml"

    [local_server.dictionaries.toml.contents]
      foo = "bar"
      baz = """
qux"""

Go structure for object stores:

type LocalServer struct {
	Backends     map[string]LocalBackend       `toml:"backends"`
	Dictionaries map[string]LocalDictionary    `toml:"dictionaries,omitempty"`
	ObjectStore  map[string][]LocalObjectStore `toml:"object_store,omitempty"`
}

type LocalObjectStore struct {
	Key  string `toml:"key"`
	Path string `toml:"path,omitempty"`
	Data string `toml:"data,omitempty"`
}

which maps to a toml like:

[local_server]

  [local_server.object_store]

    [[local_server.object_store.store_two]]
      key = "first"
      data = "This is some data"

    [[local_server.object_store.store_two]]
      key = "second"
      path = "strings.json"

or, IMO even nicer:

[local_server]

  [local_server.object_store]
    store_one = [{key = "first", data = "This is some data"}, {key = "second", path = "strings.json"}]

@Integralist
Copy link
Collaborator

Thanks @joeshaw PRs are definitely welcomed, especially for the consistency issues which are quite frustrating from a devex perspective 👍🏻

ObjectStore map[string]LocalObjectStore `toml:"object_stores,omitempty"`
Backends map[string]LocalBackend `toml:"backends"`
Dictionaries map[string]LocalDictionary `toml:"dictionaries,omitempty"`
ObjectStore map[string][]LocalObjectStore `toml:"object_store,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

@chrispoole643 were you documenting the ObjectStore in https://developer.fastly.com/reference/compute/fastly-toml/ ? I thought I recalled you doing so but couldn't see it in there at the moment. At any rate, we'll want to ensure that's kept consistent with what's here ☝🏻

@Integralist Integralist merged commit 8dd1f28 into fastly:main Nov 16, 2022
@joeshaw
Copy link
Member Author

joeshaw commented Nov 16, 2022

I opened fastly/Viceroy#206 to address it. If it's merged I'll adjust things here too.

@Integralist Integralist added the bug Something isn't working label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants