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

Breaking experience: Difference in how Lage processes dependency graph post 0.32 #161

Closed
GiriB opened this issue Aug 23, 2021 · 5 comments · Fixed by #164
Closed

Breaking experience: Difference in how Lage processes dependency graph post 0.32 #161

GiriB opened this issue Aug 23, 2021 · 5 comments · Fixed by #164

Comments

@GiriB
Copy link

GiriB commented Aug 23, 2021

We saw few breaks in our pipeline after updating from 0.29.3 to 0.32.1. We have been able to reproduce the issue locally and found out that it was because of an "ambiguity" in one of our packages - which is that one of the middle verbs in a verb dependency chain was missing.

So say for dependency like test:e2e -> bundle -> build; if a package.json has test:e2e and build defined, but not bundle

  • lage@0.29.3 would correctly run build then test:e2e (skipping bundle)
  • lage@0.32.1 would just run test:e2e (skipping build and bundle both)

Below are the steps to reproduce it. (Create a workspace with 2 packages)

lage.config.js

module.exports = {
  "pipeline": {
    "build": [ ],
    "bundle":["build"],
    "test:e2e": ["bundle"]
  }
}

packageA/package.json

{
  "name": "packageA",
  "version": "1.0.0",
  "scripts": {
    "build": "echo packageA:build",
    "test:e2e": "echo packageA:test:e2e"
  }
}

packageB/package.json

{
  "name": "packageB",
  "version": "1.0.0",
  "scripts": {
    "build": "echo packageB:build"
  }
}

Running with Lage 0.31
Notice how build ran for packageA even when there was no bundle. Even more surprisingly, build ran for packageB even when there is NO mention of test:e2e at all!!

C:\Users\gibhatna\Documents\demo> npx lage@0.31 test:e2e
npx: installed 395 in 30.192s
info Lage task runner - let's make it
info packageA build ▶️ start
info packageB build ▶️ start
info packageB build ✔️ done  - 1.14s
info packageA build ✔️ done  - 1.15s
info packageA test:e2e ▶️ start
info packageA test:e2e ✔️ done  - 1.02s
info 🏗 Summary
info
info [Tasks Count] success: 3, skipped: 0, incomplete: 0
info ----------------------------------------------
info Took a total of 2.46s to complete

Runing with Lage 0.32
Notice how it only runs test:e2e and does not run build even though it was present in the chain test:e2e -> bundle ( not present in package.json) -> build

C:\Users\gibhatna\Documents\workspace\test [master +5 ~0 -0 !]> npx lage@0.32 test:e2e
npx: installed 387 in 75.241s
info Lage task runner - let's make it
info packageA test:e2e ▶️ start
info packageA test:e2e ✔️ done  - 0.92s
info 🏗 Summary
info
info [Tasks Count] success: 1, skipped: 0, incomplete: 0
info ----------------------------------------------
info Took a total of 1.02s to complete

From what I see, Lage graph parsing has become stricter, which is why packages (like ours) which had missing/misconfigured verbs, were building fine with older versions of Lage (in some cases just luck! that interdependent packages were building at all!)

@kenotron Was this intentional? If yes, can we update documentation on this "strictness" that was introduced?

@GiriB GiriB changed the title Breaking experience: Difference in how Lage processes dependency graph post 0.32.1 Breaking experience: Difference in how Lage processes dependency graph post 0.32 Aug 23, 2021
@kenotron
Copy link
Member

kenotron commented Aug 31, 2021

The laxed implementation was definitely not intentional in the first place! I'm going to take a look at making a repro as a test for future fix here. I'll work to restore that functionality ASAP.

@GiriB
Copy link
Author

GiriB commented Sep 2, 2021

Thanks @kenotron ! If we believe that the strictness is the right thing to do, then that also makes sense to me. Yes, it'll be bit of work - teams would have to fix their build graphs, but I think that is a good thing.

I'll let Lage team decide what's the right future here, but either ways, adding an example in documentation to clarify what is intended and what's not would be great!!

@kenotron
Copy link
Member

kenotron commented Sep 3, 2021

So I think restoring default behavior is imperative - we can add a "--strict" or whatever to enforce many strictness things like file access, etc.

@kenotron
Copy link
Member

kenotron commented Sep 3, 2021

Update 1: alright, locally I have a e2e test written with your case! (thanks for the small repro repo)

@kenotron
Copy link
Member

kenotron commented Sep 4, 2021

Update 2: this is fixed in my latest (failed test is now green :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants