chore: package dependencies debt

[vic1707]

PR implementing: #631 (comment) in order to fix the circular dependency of #631.

From @prestist

New Structure:

translate/              ← NEW: Top-level package
  interface.go          ← Define Translator interface
  registry.go           ← Registry implementation

base/v0_7_exp/
  schema.go             ← Data structures only
  translator.go         ← Implements translate.Translator interface

config/fcos/v1_7_exp/
  schema.go             ← Wraps base schemas
  translator.go         ← Implements translate.Translator interface
  init.go               ← Registers with translate.Registry

[...]
So I would recommend that we do it in stages,

1. define the interfaces.
2. review that
3. Implement the use of the interfaces

[vic1707]

First attempt at 1. define the interfaces + 2. review that,

A translate module already existed and already exposed the various interfaces we want to move/modify, so for now I created them in a new code translator module.
Once we find a suitable new interface and I implement it, I'll be removing dead code and renaming the translator module back to translate.

What do you think of the current proposal @prestist (sorry for the multiple pings today 🙇) ? I feel like the already existing interface was good enough so I basically copy-pasted it.
Only the registry is new here 🤔.

[prestist]

@vic1707 No worries on the pings! Looking now.

[prestist]

Ah, I am sorry, yes there is an interface but we need it to do more for us. Currently its being used for "translate" only annd because of how its used we cannot use it in a way that enables us to lookup the implementation we need at runtime.

[prestist]

Yeah, this is an interface but i think it needs to be expanded.

So im pretty sure what we are hitting a cycle on is validation, and you can see that the translate has already been inverted but we need to do the same for validate I believe.

We might need more then just validate tho.

[vic1707]

ok for validate too, I think the rest will come as I implement it. It's hard to plan that much in advance 😅

[prestist]

100% agree'd. I think after we add validate and metadata we should be good atleast to start

[prestist]

I was thinking we would have

a set(register), get, list, then maybe we could add metadata to the interface.

I.e a Translator would have metadata that would be used to lookup the translator/validator you want.

[prestist]

Actually we prob, want a new type that is used in translator

Metadata

with "variant, version,ignition version, description, and maybe isexperimental"

[vic1707]

sounds good, one of my first idea was to re-use

type commonFields struct {
	Variant string         `yaml:"variant"`
	Version semver.Version `yaml:"version"`
}

as the key to the registry, I guess it can/should be the base for the new Metadata type 👍

[prestist]

Ah that would make sense. Yeah its basically the building block of what we need.

[vic1707]

Hi @prestist, sorry it took so long, lots of stuff going on right now 😓
just pushed a v2, while it's probably better, I'm not convinced it's right, the Metadata getter on the Translator doesn't feel right.

I didn't touch much on the registry as I'm not sure I understand what you meant from set(register) a top level function taking in the registry?

[prestist]

@vic1707 No worries, and there is no rush on this. I will take a look today and see what I can do to help :)

Sorry I am a bit behind on this, been feeling under the weather today.

[prestist]

@vic1707 I gave it a shot, there is likely something I am overlooking but this is my interpretation of what we need.

You can see the registry is basically a map, which we can register all of our implementations on.

The next step would be to implement this interface and see where the holes are, I would probably try it with base07_exp config and lets see.

[vic1707]

@prestist thanks for your commit, I think I better understand now.
I'll try to implement it asap and see were this goes 🙃

Quick question, in the Translator interface, you did

	Validate(ctx context.Context, input []byte) (report.Report, error)

I wonder if this is the right interface, I would've gone with

	Validate(ctx context.Context, input interface{}) (report.Report, error)

Since the current methods used for validation work on the schema datastruct, not the bytes input 🤔
ie:

func (t Tree) Validate(c path.ContextPath) (r report.Report) {
	if t.Local == "" {
		r.AddOnError(c, common.ErrTreeNoLocal)
	}
	return
}

---

I feel like having

	// Parse yml into schema struct
	Parse(input []byte) interface{} // Is basically a yaml.UnMarshal wrapper?
	// From schema struct to Ignition struct
	Translate(input interface{}, options common.TranslateBytesOptions) (interface{}, report.Report, error)
	// Validates yml struct
	Validate(in interface{}) report.Report

would make more sense?
IMO, the translator's job is to expose its functionalities, the registry will be calling them and has the responsibility to call Validate (assuming we don't force users to validate at the translator level)

[prestist]

@vic1707 Great question! Sorry for the slow reply – holidays had me away.

I'm pretty sure we want to keep the proposed signature, but I could be wrong.

My thoughts:

The Validate(ctx context.Context, input []byte) (report.Report, error) signature
works at the interface level because:

• Abstraction - we hide the interface{} from the caller
• Consistency - matches patterns like TranslateBytes
• Encapsulation - the whole workflow (parse → validate → translate) lives inside the
  translator implementation, not exposed through the interface

Why not expose interface{} in the signature?

In doing that, we'd would make the caller have more complex requirements - they would need to know
the internal flow (parse → validate → translate), which breaks encapsulation and
makes the API harder to use. So instead keep the complexity in the implementation.

The main goal is to keep the interface simple, maintain encapsulation, and make the registry
easy to use.

[vic1707]

Hi, No worries, free time is also hard to find on my end (renovation work + job) 😓

Ok I think I get your point, but it raises one question

You expect for all Translators to have

func (t *FCOS0.7) Validate(ctx context.Context, input []byte) (report.Report, error) {
	yaml, err := yaml.Unmarshal(input)

	// Validation steps

	return report, nil
}

func (t *FCOS0.7) Translate(ctx context.Context, input []byte, opts Options) (Result, error) {
	report, err := t.Validate(ctx, input)
	// ...
}

Validate returns an error or a Report, not the interface{}.
That means Translate uses Unmarshal a second time itself (and that for each butane files, even imported modules)? if not where should we use the opts?

For now I don't have a better interface to submit matching your requirements 🤔

---

But, if I can cycle back to my edited proposition,

	// Parse yml into schema struct, basically a yaml.Unmarshal wrapper?
	Parse(input []byte, /*opts?*/) (interface{}, error)
	// From inner schema struct to Ignition struct
	Translate(input interface{}, options common.TranslateBytesOptions) (interface{}, report.Report, error)
	// Validates yml inner struct
	Validate(in interface{}) (report.Report, error)

Wouldn't the TranslatorRegisty be the abstraction not exposing the inner interface{}?
We would get a registry with

func TranslateBytes([]byte input, options common.TranslateBytesOptions) (Result, error) {
	t, err := // get the right translator
	// Parse into interface{}
	// Validate interface{}
	// Translate
	return res, nil
}

This maintains the Consistency, Abstraction (technically) but the encapsulation workflow moves to the registry.
It would, IMO, be beneficial to potential downstream users of the lib who would get access to all the inner workings.
tests could make use of unvalidated schemas if needed (partial ones) not relevant, but I feel like tests could be better/easier to write with this approach, the methods are more independent

[prestist]

But, if I can cycle back to my edited proposition,

	// Parse yml into schema struct, basically a yaml.Unmarshal wrapper?
	Parse(input []byte, /*opts?*/) (interface{}, error)
	// From inner schema struct to Ignition struct
	Translate(input interface{}, options common.TranslateBytesOptions) (interface{}, report.Report, error)
	// Validates yml inner struct
	Validate(in interface{}) (report.Report, error)

Wouldn't the TranslatorRegisty be the abstraction not exposing the inner interface{}? We would get a registry with

func TranslateBytes([]byte input, options common.TranslateBytesOptions) (Result, error) {
	t, err := // get the right translator
	// Parse into interface{}
	// Validate interface{}
	// Translate
	return res, nil
}

This maintains the Consistency, Abstraction (technically) but the encapsulation workflow moves to the registry. It would, IMO, be beneficial to potential downstream users of the lib who would get access to all the inner workings. tests could make use of unvalidated schemas if needed (partial ones) not relevant, but I feel like tests could be better/easier to write with this approach, the methods are more independent

Oooh, you are absolutely right, yes, the Registry should be the abstraction layer, not the Translator interface itself, I think that approach should work well. Sorry for the initial push back. I was a bit focused on the wrong parts. The double unmarshaling sounds terrible lol.

In summary, yeah, I think your edited approach would be correct and should get us there.

[vic1707]

happy I pushed back a little 😄
I should get some free time over the weekend to implement one translator, we'll see how this goes