Module with declare and import

[wildum]

New modules with import and declare keywords.

Old modules are left untouched by the changes.

Fixes #5547

Note for reviewers

This PR is pretty big and not easy to review but we wanted to have most of the functionalities on one branch to be sure that the approach was the correct one. Follow-up PRs will be made to handle nits, missing functionalities, and refactors which are no blockers for the PR.

Description

A description of the functionality can be found here.

Most relevant code changes:

• ComponentNode has been renamed BuiltinComponentNode
• git logic of the module.git has been moved to a util package to be used by new modules
• New nodes have been introduced:
  • Declare node: holds a static description of a module that has a string
  • Import node: retrieve content from a source that can be defined (HTTP, git, file). The content should contain only import and/or declare blocks. When an import node imports content containing import blocks, it will create importNode children. These children are NOT part of the graph, they are evaluated and they run within their import node parent. When a child has new content, it will notify its parent recursively till the new content reaches the root import node. The root importConfigNode holds all the declare definitions from its children and is responsible for notifying its dependants via the graph for re-evaluation.
  • Custom component node: manages a module. The content of the module will be given by an import node or a declare node.
    -Previously only componentNodes could have dependants. Now customComponentNode and importConfigNodes can also have dependants. They implement the interface NodeWithDependants.
• The interface ComponentNode is implemented by the BuiltInComponentNode and the CustomComponentNode to handle both types together in the loader
• Another new interface is the ComponentInfo interface which is used by the flow_components.go to provide information on components (previously this was restricted to the struct ComponentNode). This allows other nodes like declareComponentNode and importConfigNode to appear in the UI.
• I added a few tests via module_declare_test.go, module_import_test.go and the integration tests. This is a start, we probably want to add more, especially on the error-handling part. But this could come via a separate PR.
• The old module code is left mostly untouched. I don't expect this PR to bring any harm to existing configs.

TODO (possibly in following PRs)

• import string
• import directory for import file and import git
• more/better testing
• refactor the integration tests to reduce redundancy
• explore the possibility to add a Content function to the BlockStmt in the river repo to avoid redundancy in the Declare struct
• docs
• the agent panics when the user clicks on the info button in the UI of a custom component because "river/encoding/riverjson: can only encode struct values to bodies, got map". This is due to the fact that the new modules use the map directly and not the export type. It seems to refer to "UI API fails when viewing component which exports a map block #3244"

PR Checklist

• CHANGELOG.md updated
• Documentation added
• Tests updated

[tpaschalis]

Hey there 👋 Looks like this is going to require a new cup of coffee for the day 😅

I realize it might not be a simple task to split this in logical increments, but could we offer some guidance here to help people who haven't followed development take a closer look?

• What are the high-level parts to review?
• Anything to pay special attention to or big ideas to keep in mind?
• Any areas you think warrant a closer look so we don't miss anything crucial?
• Any starting points for people wanting to try this out?

[mattdurham]

Added this to my todo today.

[rfratto]

First off, thank you for working on this! This is a lot closer to being mergeable compared to the first few efforts, but I still have some comments that I would like to see resolved, and I've done a first round of review covering those.

A brief summary of my findings:

• I'm concerned that everyone is working with a different set of definitions for what a new module is, and I think we need to have a chat offline as a group to get aligned on this.

• I'm worried there's too many differences in between native/non-native components and local/non-local modules that is leaking into the Loader implementation. Refactoring to make these constructs behave more identically and remove specialized knowledge from the Loader is likely in order.

• I find the amount of code added for passing around Flow config strings fairly alarming; we should make changes to convert those strings into ASTs as soon as we can and pass the AST around instead.

More detail about the above (and some other small comments and nits) are in the comments below.

I'll do a second final review round after the big bullet points here are cleared up.

[rfratto]

Can we not remove this field? I think it's an important part of this API to be able to know what the arguments/exports types of a builtin component are, as this will come into play in the future when we explore plugins and other tooling around components.

Keeping this the same also means we'll have even more consistency between native and non-native components, which I find ideal.

[wildum]

But the registration does not have the types? It has "Args Arguments" and "Exports Exports" which are already inside of the Info struct as "Arguments Arguments" and "Exports Exports". Removing the Registration field removes this repetition and the "Build" function (which should not be there because retrieving info about a particular instance of a component to create a new component of the same type should not be a correct way to create a component)

[rfratto]

But the registration does not have the types?

The registration does have the types; it holds the zero value for the arguments and exports types of a given component, and it's used for unmarshaling. In the future, those fields could also be used for additional validity checking without instantiating components (e.g., #3844).

(which should not be there because retrieving info about a particular instance of a component to create a new component of the same type should not be a correct way to create a component

I agree, but I also didn't suggest that the registration from this API would be the way to construct new components; just for the full set of information associated with a component.

Let me think about this a bit. This is a public API, which I think we need to be cautious about changing. I need to consider what the future implications are for not including the registration in this API, particularly around whether that limits us with the web UI or any other future services.

We should change APIs deliberately and not to make another feature easier. So if we're going to change this API, we should make sure it's the right change.

[wildum]

In builtin component, the args and exports are initialized by the registration when the node is created. That means that they also have the types and that we don't need the registration object.
Also registration is currently something specific for the builtin components. It is not used by the custom components so that would require some workaround to keep it in the component_provider info

[thampiotr]

I went through only part of this PR so far. I think we need to put more work into making this readable - I'm worried it will make maintenance in the future much harder. Let's start with adopting the new naming convention and adjusting the logic around nested imports as discussed recently. Let me know when it's ready to review further!