-
Notifications
You must be signed in to change notification settings - Fork 71
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
RFC for exporting images in parallel #291
Conversation
Maintainers, As you review this RFC please queue up issues to be created using the following commands:
Issues(none) |
Signed-off-by: Woa <me@wuzy.cn>
Hi @ESWZY! Thank you so much for working on this proposal and the RFC it is really awesome! My suggestions:
The new flags, at the end, will require a new API spec, so you can also put them under the Spec. Changes section, but that is something can be done later once you get consensus from the community around the feature. |
Signed-off-by: Woa <me@wuzy.cn>
Thanks for your suggestion! I added a proposal flag to this PRC and made a preliminary attempt in lifecycle#1167. |
# OR | ||
|
||
> /cnb/lifecycle/exporter -app cr1.example.com/foo:app -parallel | ||
|
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 would suggest adding a WARN
message to the end-user to indicate that using the parallel
flag without exporting a cache image has no effect.
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.
This makes sense, I agree with you.
- How to allow users to choose the export method? environment variable? Or a parameter of the creator? | ||
- Does it also need to be specified in the pack tool? | ||
- Should this feature be enabled by default? | ||
|
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.
From my point of view:
-
How to allow users to choose the export method? environment variable? Or a parameter of the creator?
I like how you are proposing it, with the flag the env variable -
Does it also need to be specified in the pack tool?
Probably, once we get the RFC approved, we can create the issue onpack
to expose the behavior for end-users -
Should this feature be enabled by default?
I'd say no to avoid unexpected behaviors to people consuming the lifecycle.
Awesome work @ESWZY! I really like this RFC |
Signed-off-by: Woa <me@wuzy.cn>
[-layout] \ # sets <layout> | ||
[-layout-dir] \ # sets <layout-dir> | ||
[-log-level <log-level>] \ | ||
[-parallel] \ # sets <parallel> |
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.
Suggestion from working group - when these flags are combined in the creator, -parallel-export
might make more sense. However -parallel-export
might look funny as a flag to the exporter.
Perhaps -parallel-export
as a flag to the creator and -parallel
as a flag to the exporter? I don't have a strong opinion either way, but thought I'd surface this idea.
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.
@ESWZY @jabrown85 perhaps you could opine here? Then I'll move this one back into voting. We already have enough approvals.
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.
At first I wanted to use this name, but I saw that the other flags were very short, so I chose a similar format.
But I think an informative name is very helpful for those who are new to this field.
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 think both are OK, as long as one can understand the meaning of this field by reading help messages.
Moved to voting, as this is a sub-team RFC and has approvals from both implementation maintainers I think we can merge it early cc @jabrown85 |
We already have some discussion here: lifecycle#1167.
Readable