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

[bug] Breaking change for dependent targets in v1.30.0 #1736

Closed
dudicoco opened this issue Nov 27, 2024 · 3 comments
Closed

[bug] Breaking change for dependent targets in v1.30.0 #1736

dudicoco opened this issue Nov 27, 2024 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@dudicoco
Copy link

Describe the bug

The behavior of moon ci triggering of affected targets was changed in v1.30.0.

There are two issues here:

  1. While this is partially a desired change in my view as it resolves [bug] Dependency chains are not respected in moon ci #1624, it is nonetheless a breaking change and should have been introduced behind a flag or mentioned in the release notes.
  2. It introduced a side effect of affected dependencies of a target triggering their own dependents, which is undesired in my view.

Steps to reproduce

Full reproduction is available in this branch

  1. Use moon v1.29.4
  2. Make a change to apps/backend/app-01/package.json
  3. Commit
  4. Run moon ci --base HEAD~1:
$ moon --version        
moon 1.29.4

$ moon ci --base HEAD~1
▪▪▪▪ Gathering touched files
  apps/backend/app-01/package.json
▪▪▪▪ Gathering potential targets
  app-01:install
  app-02:install
▪▪▪▪ Generating action graph
Target count: 2
Action count: 6
▪▪▪▪ Running tasks
installing
▪▪▪▪ root:install
▪▪▪▪ root:install (11ms)
app-01:install | test
▪▪▪▪ app-01:install (cached, 5aba2e13)

 SUMMARY 

pass SyncWorkspace (46ms)
skip SetupToolchain(system) (skipped, 5ms)
pass SyncProject(system, root) (5ms)
pass SyncProject(system, app-01) (5ms)
pass RunTask(root:install) (15ms)
pass RunTask(app-01:install) (cached, 40ms, 5aba2e13)

 STATS 

Actions: 5 completed (1 cached), 1 skipped
   Time: 113ms
  1. Switch to moon v1.30.0
  2. Run moon ci --base HEAD~1:
$ moon ci --base HEAD~1
▪▪▪▪ Gathering touched files
  apps/backend/app-01/package.json
▪▪▪▪ Gathering potential targets
  app-01:install
  app-02:install
▪▪▪▪ Generating action graph
Target count: 2
Action count: 8
▪▪▪▪ Running tasks
▪▪▪▪ root:install
  root:install | installing
▪▪▪▪ root:install (9ms)
app-01:install | test
▪▪▪▪ app-01:install (cached, 5aba2e13)
app-02:install | test
▪▪▪▪ app-02:install (cached, 009f8d7a)

 SUMMARY 

pass SyncWorkspace (33ms)
skip SetupToolchain(system) (skipped)
pass SyncProject(system, root) 
pass SyncProject(system, app-02) 
pass SyncProject(system, app-01) 
pass RunTask(root:install) (11ms)
pass RunTask(app-01:install) (cached, 30ms, 5aba2e13)
pass RunTask(app-02:install) (cached, 39ms, 009f8d7a)

 STATS 

Actions: 7 completed (2 cached), 1 skipped
   Time: 86ms

As you can see in the second run with moon v1.30.0, the root:install task which was triggered because it's a dependency of the app-01:install task, triggered its own dependents - app-02:install.

Expected behavior

I would want a way to control if a dependency triggered by upstream tasks wii trigger its own dependents.

@milesj
Copy link
Collaborator

milesj commented Nov 27, 2024

@dudicoco We don't run dependents for the dependencies of a task. The logic is here: https://github.com/moonrepo/moon/blob/master/crates/action-graph/src/action_graph_builder.rs#L392

We create a new RunRequirements struct with default values, which sets the dependents field to false.

I think something else may be going on, can you paste the --log trace output? Can you also paste the output of moon task-graph app-01:install --dot?

@dudicoco
Copy link
Author

dudicoco commented Dec 8, 2024

@milesj after upgrading to v1.30.3 this has been resolved.
Most likely fixed by #1746

Perhaps some regression tests could be added to make sure behaviors don't change from version to version unless they are marked as breaking changes?
This will help improve the stability of moon for production use.

@milesj
Copy link
Collaborator

milesj commented Dec 8, 2024

Cool, luckily that was the fix. I'll look into adding more tests.

@milesj milesj closed this as completed Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants