-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
feat: split js_image_layer into fine grained layers #1712
Conversation
ed8bd25
to
6727245
Compare
I think we should put node and node_modules into same layer as those are less likely to change. |
hmm how about; |
1e4fb86
to
95db098
Compare
They could; node_modules_links include 1p links but won't change as much as the actual contents of the 1p links. Is there a downside to more fine grained layers? |
95db098
to
87ed10f
Compare
Other than network round trip not really, more the better, but i am afraid that we are splitting it too much that it will be confusing. |
Its transparent for users of rules_oci tho right since |
Makes sense, i guess this is fine, fine grained layering will give users more power. |
0f35785
to
b66324b
Compare
Completely agree with fine-grained layering along these lines - it should save a lot of bandwidth and time for orgs that have 100+ services. Thanks @gregmagolan! |
I think this PR has a bad rebase, @gregmagolan could you fix that before i can take a look? |
b66324b
to
a1823d0
Compare
Ahh yes, the base branch was rebased which messed up the commits. All clean again now. |
Resolves #1641.
This change splits the output of js_image_layer into 5 layers:
node
layer contains Node.js and the node fs patchespackage_store_3p
layers containers all 3p deps in the.aspect_rules_js
package storepackage_store_1p
layers containers all 1p deps in the.aspect_rules_js
package storenode_modules
layer contains allnode_modules/foo
symlinks into the packages toreapp
layer contains everything that doesn't fall into any of the above layers