-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add build steps for building tarball #567
Conversation
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.
Just a small comment
|
||
@Override | ||
public Void call() throws ExecutionException, InterruptedException { | ||
ImmutableList.Builder<ListenableFuture<?>> dependenciesBuilder = ImmutableList.builder(); |
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.
Aside: we should probably figure out a good way to organize all the duplications of the code like this in all the steps.
|
||
// Build the image to a tarball | ||
buildConfiguration.getBuildLogger().lifecycle("Building image to tar file..."); | ||
if (Files.exists(outputPath.getParent())) { |
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.
What is this doing? If the file exists, wouldn't createDirectories
not do anything?
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.
I added it just in case because I wasn't sure - I should really get into the habit of reading documentation on methods I'm not 100% sure of.
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.
Ah wait now I know why I did it - createDirectory
throws an exception if the directory exists, createDirectories
doesn't. So I guess I got the two mixed up.
|
||
// Build the image to a tarball | ||
buildConfiguration.getBuildLogger().lifecycle("Building image to tar file..."); | ||
Files.createDirectories(outputPath.getParent()); |
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.
Do we want to be generating all the parent directories though? Perhaps we should only generate the file in an existing directory?
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.
In our use case, yes, the directory should already exist since we're putting it in the build output. But I imagine if this were eventually configurable (which I guess it already is from the library perspective), then it'd be a better experience to generate all the directories the user specifies rather than throwing an exception if the directory structure isn't already setup.
Part of #514