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

Included task that includes a task that includes a common task fails due to a "cyclic loop" #122

Closed
Racer159 opened this issue Jun 18, 2024 · 2 comments · Fixed by #161
Closed
Labels
bug 🐞 Something isn't working

Comments

@Racer159
Copy link
Contributor

Environment

Device and OS: popOS 22.04
App version: 0.2.1
Kubernetes distro being used: N/A
Other: N/A

Steps to reproduce

  1. Create the following with a common import:

tasks.yaml

includes:
  - lint: https://raw.githubusercontent.com/defenseunicorns/uds-common/main/tasks/lint.yaml
  - import: import.yaml

tasks:
  - name: default
    actions:
     - task: lint:yaml
     - task: import:lint

import.yaml

includes:
  - lint: https://raw.githubusercontent.com/defenseunicorns/uds-common/main/tasks/lint.yaml

tasks:
  - name: lint
    actions:
     - task: lint:yaml
  1. Run maru run

Expected result

Linting successfully runs twice

Actual Result

Maru complains about an "include loop"

Visual Proof (screenshots, videos, text, etc)

image

Severity/Priority

Medium - this forces unnecessary copying of tasks if a common task is needed in many places

Additional Context

This was found through the implementation of the GitLab runner tests where a deploy task happens in a parent and then is reused in a child to redeploy the app.

@Racer159 Racer159 added possible-bug 🐛 Something may not be working bug 🐞 Something isn't working and removed possible-bug 🐛 Something may not be working labels Jun 18, 2024
@Racer159
Copy link
Contributor Author

Diagram of what is going on:

task.yaml
|  |
|  |--> include.yaml
|            |
|            | ---> lint.yaml
|                     ^
|---------------------|

Racer159 added a commit that referenced this issue Oct 3, 2024
…athing (#152)

## Description

Marking this as a feat since it is a change in functionality that may
not be expected - really it fixes variables not being loaded when
calling an included task directly and fixes pathing on remote URLs that
then import local files.

> [!CAUTION]
> ⚠️ **BREAKING CHANGE** - This has the potential to be breaking
depending on where things are defined in your setup - if you are
currently relying on variables or local pathing from remote tasks to
_not_ work this may break your setup. Also
#122 may be more
pronounced in complex setups because we are now able to load in more
tasks.

## Related Issue

Fixes #133

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [X] Test, docs, adr added or updated as needed
- [X] [Contributor Guide
Steps](https://github.com/defenseunicorns/maru-runner/blob/main/CONTRIBUTING.md)
followed

---------

Co-authored-by: Eric Wyles <23637493+ericwyles@users.noreply.github.com>
@mjnagel
Copy link

mjnagel commented Oct 15, 2024

I just came across this issue or maybe an adjacent one. I initially posted a full reproduction but decided to minimally reproduce with some tasks that are easy to run wherever... minimal reproduction of the same flow without my unique tasks:

tasks.yaml:

includes:
  - test: test.yaml

tasks:
  - name: test-test
    actions:
      - task: test:test

test.yaml:

includes:
  - utils: utils.yaml
  - thing: thing.yaml

tasks:
  - name: test
    actions:
      - task: thing:echo
      - task: magic

  - name: magic
    actions:
      - task: utils:echo

thing.yaml:

includes:
  - utils: utils.yaml

tasks:
  - name: echo
    actions:
      - task: utils:echo

utils.yaml:

tasks:
  - name: echo
    actions:
      - cmd: echo hello world

Calling uds run test fails, but interestingly uds run test:test and uds run -f test.yaml test do not fail. Modifying thing.yaml to "includes" util instead of utils resolves the cyclic check failure.

Racer159 added a commit that referenced this issue Oct 17, 2024
## Description

Marking this as a fix to #122 since it will fix the issue in most cases
and it allows loops since those can be valid constructs in some cases.
Long term we still need to refactor this code.

## Related Issue

Fixes #122

## Type of change

- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [X] Test, docs, adr added or updated as needed
- [X] [Contributor Guide
Steps](https://github.com/defenseunicorns/maru-runner/blob/main/CONTRIBUTING.md)
followed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants