Skip to content

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Jul 21, 2023

Summary

After chatting with @Lagoja we decided to remove packages and match from plugin schema before we release custom plugins. The reason being:

  • packages is poorly defined and not a great experience. It was added so we could build plugins that install custom flakes (for php, haskell, etc) and replace planners. It was renamed to __packages and will not be externally documented. In very near future we plan on adding suggested_packages (final name TBD) which informs the user of any packages needed for a plugin and allows the user to install if needed.
  • match is designed to automatically add built-in plugins if certain packages are installed. This functionality doesn't make sense for custom plugins. This field was moved into a hardcoded map. Another benefit of removing this from schema is that we can put the plugins into the plugin github repo.

cc: @Lagoja

How was it tested?

  • unit tests
  • manual smoke testing (installed php)

@mikeland73 mikeland73 requested review from loreto and mohsenari July 21, 2023 21:26
Copy link
Collaborator

@mohsenari mohsenari left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of __packages I think it's even more confusing than packages but if we're going to replace it with something else like suggested packages (we need a clearer name for this field), then it's fine for now.
But I'd say if we end up releasing a devbox version with __packages present, we should mention it in the docs with a note that we are planning on changing this.

regexp.MustCompile(`^postgresql(_[0-9]+)?$`): "postgresql",
regexp.MustCompile(`^python[0-9]*(Full|Minimal|-full|-minimal)?$`): "python",
regexp.MustCompile(`^redis$`): "redis",
regexp.MustCompile(`^j?ruby([0-9_]*[0-9]+)?$`): "ruby",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this list is small enough but in future iterations we should define a map of plugin name and the regex string somewhere else and do a for loop here

@mikeland73
Copy link
Contributor Author

I'm not a big fan of __packages

understore or double underscore are used to indicate something is private. So the goal is really to tell plugin developers not to use this. I'm not sure we should even document it.

Some possible solutions:

  • If we really want to make it private we could use different structs for built-in vs non built-in. And even then, using __packages helps anyone inspecting built-ins understand that the field is private.
  • Since built-ins only install custom flakes (never nix packages), we could rename this field to flakes or local_flakes which allows the plugin to create a flake and install it.

@mikeland73 mikeland73 merged commit 2f0e8eb into main Jul 21, 2023
@mikeland73 mikeland73 deleted the landau/refactor-plugins branch July 21, 2023 22:37
@mohsenari
Copy link
Collaborator

mohsenari commented Jul 29, 2023

@mikeland73 I understand this better now. I agree packages was not a good name, but I also think __pakcages is not a good name either, the double underscore indicating private is true, but this is in a json file which is why I was confused and didn't think of private field naming convention seeing it. I like the local_flakes and IMO that's a better alternative. So if we end up making changes to this, I suggest going with local_flakes and inform users who are creating custom plugins they can use that field too.

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.

2 participants