Skip to content

feat(core/pipeline): allow dependency across source/conditions/targets#2800

Merged
olblak merged 26 commits intomainfrom
lp/cross-type-deps
Oct 17, 2024
Merged

feat(core/pipeline): allow dependency across source/conditions/targets#2800
olblak merged 26 commits intomainfrom
lp/cross-type-deps

Conversation

@loispostula
Copy link
Copy Markdown
Contributor

@loispostula loispostula commented Oct 7, 2024

Overview

This pull request introduces support for dependencies across sources, conditions, and targets, making the dependency model more flexible.
Current Behavior:

  • Sources can depends on:
    • Other sources
  • Conditions can depends on:
    • Other conditions
    • One source (by sourceID)
  • Targets can depend on:
    • Other targets
    • Either:
      • All conditions
      • A subset of conditions
    • One source (by sourceID)

New Behavior:

With this change, sources, conditions, and targets can depend on any other resource type. This is a stepping stone towards resolving issues like issue #2718 and issue #159.

Implementation

To achieve this, I've extended the current dependsOn specification. It can now be defined using the format:

(resourceType#)resourceId(:booleanOperator)

This change allows merging the three resource-specific dependency Directed Acyclic Graphs (DAGs) into a single unified DAG. The DAG is built using a custom node type:

type Dependency struct {
    ID       string
    Operator string
}

type Node struct {
    ID                string
    Category          string
    DependsOn         []Dependency
    DependsOnChange   bool
    Result            string
    Changed           bool
}

To ensure backward compatibility, the existing dependencies (i.e., DependsOn) are preserved. In addition, the sourceID is passed as a dependency for conditions and targets, and conditionIDs are fed into the targets as dependencies in the DAG.

A traversal of the DAG using DescendantsFlow processes each resource in the correct order once its dependencies are completed.

Expected Impact

This PR is expected to be a no-op, meaning that while the core pipeline logic has been updated, there should be no impact on the output or behavior of existing pipelines.

updatecli manifest show -graph

In order to help debugging the dependencies tree,

I' ve added an option to the show command to display the DAG in graphviz format, this allows to get interesting information on a pipeline:

  • e2e/updatecli.d/success.d/temurin/sources.yaml:
    e2e/updatecli.d/success.d/temurin/sources.yaml
  • e2e/updatecli.d/success.d/chained.yaml:
    e2e/updatecli.d/success.d/chained.yaml
  • e2e/updatecli.d/success.d/http.yaml:
    e2e/updatecli.d/success.d/http.yaml

@loispostula loispostula marked this pull request as ready for review October 8, 2024 21:37
@loispostula loispostula requested review from dduportal, lemeurherve and olblak and removed request for dduportal and olblak October 9, 2024 15:35
@olblak olblak self-assigned this Oct 11, 2024
olblak and others added 4 commits October 12, 2024 21:53
@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 12, 2024

Thanks for the pullrequest, this one is really awesome but I need a bit more time to review it properly, I am adding commits as I am reviewing it

@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 12, 2024

I am having many warning messages like

call of append copies lock value: github.com/updatecli/updatecli/pkg/core/pipeline.Pipeline contains sync.Mutex

Comment thread pkg/core/config/main.go
@loispostula
Copy link
Copy Markdown
Contributor Author

@olblak hiw did you get those warning? Could you share a pipeline or something so that I can investigate?

@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 14, 2024

Note for myself as it don't have to be done in this pullrequest but I am wondering if you can generated the graphviz graphs directly using https://github.com/goccy/go-graphviz

@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 14, 2024

@olblak hiw did you get those warning? Could you share a pipeline or something so that I can investigate?

image

The problem is well explained on https://stackoverflow.com/questions/76648255/how-to-range-over-a-slice-of-structs-that-contain-mutexes-in-go

Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 14, 2024

I think this commit fixes the mutex issue
https://github.com/updatecli/updatecli/tree/55b2fc247631de801646687c87c659d5fde90c8f

@olblak olblak added enhancement New feature or request core All things related to Updatecli core engine labels Oct 14, 2024
@loispostula
Copy link
Copy Markdown
Contributor Author

loispostula commented Oct 15, 2024

Note for myself as it don't have to be done in this pullrequest but I am wondering if you can generated the graphviz graphs directly using https://github.com/goccy/go-graphviz

That's a good point. I think having a graph functionality could also be useful on generating some summary, with what ran and what didn't. But then maybe dot is not the ideal choice and we should use mermaid-js so that it's easy to but in github?

But this should be new issues / new pr. Here the goal is really to help debug and visualize dependencies

@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 16, 2024

But this should be new issues / new pr. Here the goal is really to help debug and visualize dependencies

I think I would move the graph option behind the experimental flag so we can change it later.
Theh current pullrequrest is already big enough that I wouldn't add new content to it

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Copy Markdown
Member

olblak commented Oct 16, 2024

I just fixed the updatecli manifest show so I think we are good to merge this pullrequest

olblak
olblak previously approved these changes Oct 16, 2024
Signed-off-by: Olblak <me@olblak.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core All things related to Updatecli core engine enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants