-
Notifications
You must be signed in to change notification settings - Fork 33
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: Support legacy 'import' directive and use 'imports' for future #543
feat: Support legacy 'import' directive and use 'imports' for future #543
Conversation
This wont work right now, I didn't adjust any of the tests, but this is the 'import -> /stacker' and 'imports -> /stacker/imports' change that I spoke with people about this week. |
Would also add a "Warning" that this is to be deprecated. |
de6ea75
to
1d7c9d6
Compare
done. |
b5ad040
to
be926f4
Compare
be926f4
to
17784f3
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
+ Coverage 53.57% 54.29% +0.71%
==========================================
Files 64 64
Lines 7473 7505 +32
==========================================
+ Hits 4004 4075 +71
+ Misses 2798 2737 -61
- Partials 671 693 +22 ☔ View full report in Codecov by Sentry. |
7a5d2fa
to
844d857
Compare
@rchincha or @hallyn I'd appreciate feedback here. This seems like a win to me. Basically
Note, it does break things that had moved to 1.0.0-rc5+ (where 'import' imported into /stacker/imports). Consumers moving from 1.0.0-rc5+ will have to change 'import:' to 'imports:' in stacker files. Consumers moving from < 1.0.0-rc5 should not need any changes. Also note, we get a warning to console about deprecation of the 'import'. I would personally like to change that warning to put a stick in the mud saying "No stacker release after 2024-01-01 will support 'import'". That gives anyone picking this up a year to change and after that we can remove the old support. |
844d857
to
34a27f8
Compare
This changes the behavior of the 'import' target to behave as it did before the breaking change that moved imports into /stacker/imports/ So now, if the stacker file uses 'import', then imports will be placed in /stacker. If the stacker file uses 'imports' (plural) then they will be placed in /stacker/imports. What we actually get in both cases is a consistent set of binds being done into a different "base". Either /.stacker (legacy) or /stacker (new). stacker -> <base>/bin/stacker imports -> <base>/imports runscript -> <base>/imports/.stacker-run.sh artifacts -> <base>/artifacts In the legacy case, the imports are also bind-mounted into /stacker Signed-off-by: Scott Moser <[email protected]>
This just changes all the existing tests to test 'imports' rather than 'import'. Then it adds one test for 'import' explicitly and within the same stacker file. Test test/cache.bats test "can read previous version's cache" is skipped as the old version can't build a stacker file in the test because it uses 'imports'. Signed-off-by: Scott Moser <[email protected]>
34a27f8
to
745b2bb
Compare
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.
lgtm
This changes the behavior of the 'import' target to behave as it did before the breaking change that moved imports into /stacker/imports/
So now, if the stacker file uses 'import', then imports will be placed in /stacker. If the stacker file uses 'imports' (plural) then they will be placed in /stacker/imports.
What we actually get in both cases is all the binds being done into either /.stacker (legacy) or /stacker (new). In the legacy case, the imports are also bind-mounted into /stacker
Legacy (for those that used 'import:')
new (for those that use 'imports:')
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.