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

[config] Refactor config to prepare for imports #1817

Merged
merged 10 commits into from
Feb 19, 2024
Merged

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Feb 16, 2024

Summary

Currently we use config fields on the Config struct directly. This makes it really hard to track what we use to modify the file vs what we are using as the representation of the config. This was not a problem in the past because the config and the devbox.json file are one in the same. Since we are hoping to introduce imports, we need to make it more clear what each field/function refers to.

In this PR, I split config into 2 structs:

  • configFile which represents a single JSON file and is not exported directly.
  • Config a wrapper around a Root configFile and in the future possible imports. The root is exported so that we don't have to wrap every configFile field.

Note: Config has a bunch of functions that will need to be re-implemented in order to support imports.

Other changes:

  • Rename Packages struct to PackagesMutator and removed all non-mutating functions. This makes the struct more purposeful.
  • Made a few structs not-exported.
  • Moved some tests around.

Note: Github diff UI doesn't do a great job with file name changes in this PR so to make this PR easier to read I named the new Config file config2.go while keeping the previous config.go. Before merging I'll rename config.go file.go and config2.go -> config.go.

This code change should have no functional change on devbox.

How was it tested?

builds, tests

@gcurtis
Copy link
Collaborator

gcurtis commented Feb 16, 2024

Instead of making the config an interface, what if we add a separate type to represent the resolved environment? It just seems like they have different enough uses that it would be warranted. For example:

package devbox

// Config is a single devbox.json file.
type Config struct {}

func (c Config) Env() map[string]string
func (c Config) Packages() []Package
func (c Config) Imports() []string
func (c Config) SaveTo(string) error

// Environment is a computed Devbox environment after applying configuration
// and lock files.
type Environment struct {}

func Load(cfg ...Config) (Environment, error)
func (e Environment) Env() map[string]string
func (e Environment) Packages() []Package
func (e Environment) Configs() []Config

The Config is the only thing that's editable and directly represents what's in a single file. The Environment provides a view of things after configs have been merged and lock files have been applied. Speaking of lock files, have we thought about how those will apply to imports, if at all?

@mikeland73
Copy link
Contributor Author

I like the idea of 2 structs. The issue is there's actually a bunch of fields that are in the JSON that we never want to expose to users of this package. For example Env or Includes don't need to be exposed. Providing them increases the risk that a user tries to use that instead of the Environment struct (which is what they should use)

On the other hand, since the Environment struct doesn't have a JSON representation it can keep most fields unexported and compute functions dynamically.

@mikeland73 mikeland73 closed this Feb 16, 2024
@mikeland73 mikeland73 reopened this Feb 16, 2024
@mikeland73
Copy link
Contributor Author

So we could:

  • Use a simple interface for Config (and remove any functions that don't make sense)
  • Export Config fields even if we never want people to use them
  • Have 3 structs (on for JSON, one for Config, and one for Environment)

@mikeland73
Copy link
Contributor Author

Regarding lock file, I would keep that separate for now. Since we don't allow duplicate packages in imports (we only pick one of them) the lockfile can keep track of all resolves around all packages including imports (later on, imports could have their own lockfiles, but that feels lower priority)

@mikeland73
Copy link
Contributor Author

@gcurtis moved to an implementation that doesn't use interfaces and avoids exporting fields we don't want folks to use (some fields are available via the Root field. This felt like a middle ground to avoid having to wrap each field in a function).

The functions that are wrappers in `config2.go will all have future logic to pick up imports, dedup, etc so they won't just be dumb wrappers.

PTAL

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@mikeland73 mikeland73 merged commit d3c064a into main Feb 19, 2024
24 checks passed
@mikeland73 mikeland73 deleted the landau/refactor-config branch February 19, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants