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

feat: Support legacy 'import' directive and use 'imports' for future #543

Merged
merged 2 commits into from
Nov 25, 2023

Conversation

smoser
Copy link
Contributor

@smoser smoser commented Nov 11, 2023

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:')

 stacker: /.stacker/bin/stacker
 imports: /.stacker/imports -> /stacker
 runscript: /.stacker/imports/.stacker-run.sh
 artifacts: /.stacker/artifacts

new (for those that use 'imports:')

 stacker: /stacker/bin/stacker
 imports: /stacker/imports
 runscript: /stacker/imports/.stacker-run.sh
 artifacts: /stacker/artifacts

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.

@smoser
Copy link
Contributor Author

smoser commented Nov 11, 2023

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.

@mikemccracken , @hallyn , @rchincha

@rchincha
Copy link
Contributor

Would also add a "Warning" that this is to be deprecated.

@smoser
Copy link
Contributor Author

smoser commented Nov 13, 2023

Would also add a "Warning" that this is to be deprecated.

done.

@smoser smoser force-pushed the feat/legacy-import-are-back branch 2 times, most recently from b5ad040 to be926f4 Compare November 14, 2023 02:48
@smoser smoser marked this pull request as ready for review November 14, 2023 02:48
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (a34ebfa) 53.57% compared to head (745b2bb) 54.29%.

Files Patch % Lines
pkg/stacker/bom.go 0.00% 10 Missing ⚠️
cmd/stacker/bom.go 0.00% 8 Missing ⚠️
pkg/stacker/build.go 77.77% 6 Missing and 2 partials ⚠️
pkg/types/layer.go 70.00% 2 Missing and 1 partial ⚠️
pkg/stacker/grab.go 87.50% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@smoser smoser force-pushed the feat/legacy-import-are-back branch 3 times, most recently from 7a5d2fa to 844d857 Compare November 15, 2023 14:01
@smoser
Copy link
Contributor Author

smoser commented Nov 15, 2023

@rchincha or @hallyn I'd appreciate feedback here.

This seems like a win to me. Basically

  1. gets us backwards compatibility with "old stacker" (0.4.x) branch
  2. makes stacker files more consistent. the following directives in a layer are all plural: overlay_dirs, volumes, labels, generate_labels, binds, annotations. import was singular, the change makes it plural going forward.
  3. Allows users to move to newer stacker and move to new 'imports' lazily. the moves can even be done on one layer at a time (inside the same stacker file you can use 'import' for one layer and 'imports' for another). That reduces our need to maintain the 0.4.x branch.
  4. cleans up the "binds" code a bit.

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.

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 <smoser@brickies.net>
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 <smoser@brickies.net>
Copy link
Contributor

@rchincha rchincha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rchincha rchincha merged commit 2f284d8 into project-stacker:main Nov 25, 2023
11 checks passed
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 this pull request may close these issues.

2 participants