-
Notifications
You must be signed in to change notification settings - Fork 79
If target has no outputs set, and no global outputGlob set, assume no outputs #878
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
base: master
Are you sure you want to change the base?
Conversation
… outputs Assuming that every file in the target directory is an output to be cached doesn't seem correct here - if they didn't specify outputs, assume there are none instead.
9cbdad5 to
72f3c63
Compare
ecraig12345
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, but this seems like a risky change since outputs and outputGlob are both optional, and there's no config validation error if neither is defined. So it's changing from implicitly caching (probably) too much to implicitly caching nothing--neither behavior is great, but at least outside a major bump I think it's safer to preserve what's done today.
|
The current behavior seems broken to me. But it's up to you I guess. I have a workaround in place. Or are you saying I should update the pull request to do a major version bump? |
1 similar comment
|
The current behavior seems broken to me. But it's up to you I guess. I have a workaround in place. Or are you saying I should update the pull request to do a major version bump? |
I agree it's weird, but it's too significant of a change in default behavior outside a major version bump. It's also unfortunate that the doc comments inherited from backfill say the default is Probably the "right" approach in the next major version is to make |
|
One thought that comes to mind is maybe add some kind of config checking command like "lage lint-config" which you could start recommending people add to their CI flow. It could have stricter rules, like requiring these fields you mentioned. But it wouldn't break backwards compatibility because it wouldn't cause an error when running lage build.
Or perhaps you could put a version into the lage config object or a new option that sets the default for outputs as other thoughts for backcompat while addressing this footgun.
Just some thoughts... You can also just put it off
…On Jan 27, 2026 at 1:53 AM -0800, Elizabeth Craig ***@***.***>, wrote:
ecraig12345 left a comment (microsoft/lage#878)
> The current behavior seems broken to me. But it's up to you I guess. I have a workaround in place.
> Or are you saying I should update the pull request to do a major version bump?
I agree it's weird, but it's too significant of a change in default behavior outside a major version bump. It's also unfortunate that the doc comments inherited from backfill say the default is lib/**/*, but that doesn't appear to be true with the way lage sets up the config, and isn't a good assumption anyway since not everyone uses that output structure.
Probably the "right" approach in the next major version is to make cacheOptions.outputGlob and probably .environmentGlob required, since the behavior without either of those options is pretty bad/unexpected and defeats a significant part of the purpose of using lage. Lage is widely used enough that we don't want to bump the major version for just one change, but we probably will need a major bump due to Node version requirements in the near-ish future, so maybe this could be done at that time.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Assuming that every file in the target directory is an output to be cached doesn't seem correct here - if they didn't specify outputs, assume there are none instead.