Add (short term) conditional compilation to early-boot-config #1284
Conversation
This change adds a simple (short term) solution for conditionally compiling early-boot-config based on the current variant. As part of the change, local file handling (which was previously broken) is split out into its own trait implementation and only compiled into the program for "aws-dev" variants.
| #[cfg(bottlerocket_platform = "aws-dev")] | ||
| struct LocalFileDataProvider; |
There was a problem hiding this comment.
I haven't looked through the rest yet, but why restrict the file provider to aws-dev? It's there for testing in odd situations, I'd say, and odd situations can arise in any variant. Is there a reason not to allow it in other variants?
|
Tell me more about the "short term" part. 🤔 |
There was a problem hiding this comment.
One could emit aws as bottlerocket_platform and emit an additional include_file_provider=true (or something) to conditionally add the file provider. It seems like the concerns are separate whereby aws_dev is still an aws platform, and the file provider is a different concern.
That being said, what you have here gets the job done as you need, is easy to understand, and does no harm IMO.
|
|
||
| fn main() { | ||
| // The code below emits `cfg` operators to conditionally compile this program based on the | ||
| // current variant. This approach is only meant to be in use for the short term; when the |
There was a problem hiding this comment.
This caveat might be better provided with // TODO - replace this short term approach blah blah so that it sticks out as something that should be deleted when no longer true.
I think there's some hope that rethinking of variants and build logic will make this kind of thing unnecessary (or at least different) in the future. |
OK, that makes sense. I'd like to remove that phrase from the commit and pull request title, and just link over to an issue to track that if we have one. If we merge this then it could be around for a while. |
That seems like a good idea, the issue in question is probably #1260 (though the scope is quite broad currently). |
|
Closing in favor of #1288 |
Issue number:
Related to #1218
Description of changes:
Testing done:
aws-devvariant both with and without local files. The system boots and correctly uses local files if they exist.aws-k8s-1.17variant and made sure it correctly fetches user data from IMDS on a new instance.Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.