AWS Image Builder implementation #2904
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
internal
stefannica
force-pushed
the
feature/aws-image-builder
branch
2 times, most recently
from
d17d470
to
5a77dbe
Compare
stefannica
force-pushed
the
feature/aws-image-builder
branch
from
5a77dbe
to
463e94d
Compare
| """ | ||
| files = self.get_files() | ||
| extra_files = self.get_extra_files() | ||
| intermediate_fileobj: Optional[Any] = None |
There was a problem hiding this comment.
I was a little confused by this, isn't this just a bool which indicates whether we should close the fileobj? Or do we actually need a second reference to a file for some reason?
There was a problem hiding this comment.
I was merely trying to keep your original code intact and close the GzipFile object at the end, but I didn't research if this is actually needed or not. I use intermediate_fileobj to make sure I close the GzipFile object independently of and in addition to fileobj, but only in the tar.gz case. It feels a bit awkward, because it's an exception to the overall code flow in this function.
There was a problem hiding this comment.
Yep we need to create an intermediate fileobj in that case, but this variable could still be used like this instead right?
close_fileobj: bool = False
if archive_type == ArchiveType.TAR_GZ:
close_fileobj = True
# set fileobj to the intermediate file here
if close_fileobj:
fileobj.close()At least to me this is more understandable, but feel free to ignore as well
| """ | ||
|
|
||
| code_build_project: str | ||
| aws_access_key_id: Optional[str] = SecretField(default=None) |
There was a problem hiding this comment.
I remember that we recently had other stack components where we did not add these secrets on the stack component config anymore, but instead it's either a service connector or implicit auth. Do you think it makes sense to also remove that option here, which forces users to use the "right" way of authenticating their stack components (service connectors) and makes the component config less confusing with fewer attributes/choices?
There was a problem hiding this comment.
for more info, that was the same discussion regarding the AzureML orchestrator.
There was a problem hiding this comment.
Yes, good idea. In fact, this also makes it easier for me to document this component.
Describe changes
This PR implements a new stack component: an AWS Image Builder that uses AWS CloudBuild to build container images remotely.
Side Changes
AWS CloudBuild requires the docker context files to be archived as a zip file. This PR extends the Archiver base class to add zip support on top of the existing tar and tar+gzip formats.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes