Make use of the controller-provided dynamic component dependencies #463
Conversation
brandonbloom
force-pushed
the
dynamic-deps-2
branch
from
b665d01
to
da8656d
Compare
|
This needs a fair amount of testing & it is very janky. However, I'm trying to land this janky change, since it unblocks #426, which is badly hurting the UX of the main migration path to exo.hcl files. Assuming this PR does that it's supposed to do, then at least the files on disk won't be crufted up with bad dependency information. Then I'm basically going to totally rework the component system internals in order to fix a variety of bugs. I think I know how to go about that now. |
| Spec: c.Spec, | ||
| }) | ||
| if err != nil { | ||
| ws.logEventf(ctx, "error getting %q deps: %v", c.Name, err) |
There was a problem hiding this comment.
Why isn't this considered a fatal error?
There was a problem hiding this comment.
I'll add a comment. I think that if it were a fatal error, then it would be impossible to stop components that have broken provider implementations or partially invalid specs that fail dependency analysis.
internal/core/server/workspace.go
Outdated
| @@ -599,6 +647,7 @@ func (ws *Workspace) RenameComponent(ctx context.Context, input *api.RenameCompo | |||
|
|
|||
| func (ws *Workspace) RefreshComponents(ctx context.Context, input *api.RefreshComponentsInput) (*api.RefreshComponentsOutput, error) { | |||
| query := makeComponentQuery(withRefs(input.Refs...)) | |||
| // TODO: Recompute dependencies on refresh. | |||
There was a problem hiding this comment.
I'm not quite clear why. Is this because we ignore a failure to get dependencies on apply?
There was a problem hiding this comment.
I'll expand the comment. The main reason is because the analysis for dependencies in theory may be out of date, either because the provider implementation changed or because it depends on some non-deterministic data (none do currently), so the idea is that "refresh" should fixup and busted state as best it can.
| @@ -154,29 +153,6 @@ func (c *Component) Analyze(ctx *AnalysisContext) { | |||
| } else { | |||
| c.Spec, _ = AnalyzeString(ctx, specAttr.Expr) | |||
| } | |||
|
|
|||
| depsAttr := content.Attributes["depends_on"] | |||
There was a problem hiding this comment.
Don't we still need this? How otherwise does a user explicitly define a dependency? How would this docker compose file work?
services:
t0:
image: bash
command: "sleep infinity"
t1:
image: bash
command: "sleep infinity"
depends_on:
- "t0"There was a problem hiding this comment.
depends_on works via #436 now - This is simply removing the ability to provide extra/explicit dependencies in the manifest, which I decided isn't really an important feature yet (and if it becomes one, might have to work differently, merging dependencies with the spec analysis, or similar).
brandonbloom
force-pushed
the
dynamic-deps-2
branch
from
da8656d
to
695948a
Compare
brandonbloom
force-pushed
the
dynamic-deps-2
branch
from
695948a
to
6920bc9
Compare
|
I did some testing on this and ran in to a problem where dependency resolution can fail in a non-deterministic way because of the janky way in which Apply works now. In short, we delete & recreate things, and then during that interim we might try to resolve names, but the components have been temporarily deleted. This is pretty problematic, and just more evidence that we need to do a major overhaul of the reconciliation engine. |
|
This is back to draft, since it's broken. Going to work on overhauling the internals to make this actually easier to get working right. |
|
Closing in favor of ongoing work in #543 |
This is a follow-up to #436 and is in service of #426.