fix: disable injectable-pipe migration #30180
Conversation
crisbeto
added
type: bug/fix
action: review
AndrewKushnir
added
the
action: presubmit
AndrewKushnir
added
the
target: patch
crisbeto
force-pushed
the
injectable-pipe-disable
branch
2 times, most recently
from
43d0780 to
56aa0ef
Compare
There was a problem hiding this comment.
Lgtm but can you please extend the commit message to contain more details. The problem is that we are not yet sure if this is the right solution for ivy and if it is we need to do much more to roll out this change: e.g. update style guide, scaffolding schematics, docs and tutorial, etc etc. So it's better to wait until we are sure that we want this change and then make a comprehensive plan on how to roll it out.
IgorMinar
added
action: cleanup
|
@crisbeto Thanks for putting this together so quickly. |
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc. Context for the new `test-migrations.json`: since we use the `migrations.json` both for the real migrations and for tests, it doesn't allow us to disable a schematic, but continue running its tests. This change adds the test-specific file so that we can continue running the `injectable-pipe` tests, even though the schematic itself is disabled.
crisbeto
force-pushed
the
injectable-pipe-disable
branch
from
56aa0ef to
b183a44
Compare
|
Updated the commit message and PR description with more context. |
crisbeto
removed
the
action: cleanup
There was a problem hiding this comment.
Ideally there would have been only a new collection for the injectable-pipe schematic in order to run tests without shipping it. Using the real migration collection allows us to validate that the schematics (which are intended to run publicly) are wired up correctly.
I don't mind too much though. LGTM 😄
|
Presubmit #2 (after additional changes) |
AndrewKushnir
added
hotlist: release-blocker
and removed
action: presubmit
|
@devversion I can see it going either way. I did it this way, because it's more consistent. |
|
@crisbeto True it's more consistent for the tests, but given that it means that we need to duplicate schematic collection entries and we no longer test the publicly enabled migrations with the actual Doesn't have to be part of this PR as it already had various presubmits.. |
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc. Context for the new `test-migrations.json`: since we use the `migrations.json` both for the real migrations and for tests, it doesn't allow us to disable a schematic, but continue running its tests. This change adds the test-specific file so that we can continue running the `injectable-pipe` tests, even though the schematic itself is disabled. PR Close #30180
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc. Context for the new `test-migrations.json`: since we use the `migrations.json` both for the real migrations and for tests, it doesn't allow us to disable a schematic, but continue running its tests. This change adds the test-specific file so that we can continue running the `injectable-pipe` tests, even though the schematic itself is disabled. PR Close angular#30180
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Disables the injectable pipe migration until we can decide whether this is the right solution for Ivy. Rolling it out properly will involve a more detailed plan and more changes like updating the styleguide, scaffolding schematics etc.
Context for the new
test-migrations.json: since we use themigrations.jsonboth for the real migrations and for tests, it doesn't allow us to disable a schematic while continuing to run its tests. This change adds the test-specific file so that we can continue running theinjectable-pipetests, even though the schematic itself is disabled.