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

[WFMP-174] New image goal to build (and push) application image #253

Merged
merged 1 commit into from Sep 20, 2022

Conversation

jmesnil
Copy link
Member

@jmesnil jmesnil commented Sep 9, 2022

As an user, I want to build and push an application image from the plugin

https://issues.redhat.com/browse/WFMP-174

  • new image goal
  • new tests in standalone-tests
  • code is commented (and generated in the web site)
  • code is documented (with an example in the generated web site)

Signed-off-by: Jeff Mesnil jmesnil@redhat.com

@jmesnil jmesnil force-pushed the WFMP-174_image_goal branch 9 times, most recently from e8c4786 to d29b91c Compare September 12, 2022 13:31
@jmesnil jmesnil marked this pull request as ready for review September 12, 2022 13:59
@jmesnil jmesnil requested a review from jamezp September 12, 2022 13:59
@jmesnil jmesnil force-pushed the WFMP-174_image_goal branch 3 times, most recently from 9dd5abc to 734e008 Compare September 13, 2022 07:34
Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

Just some minor comments mostly related to documentation.

One thing that could be interesting to add too, in a later PR even, is to if the password is empty prompt the user for one. We do something like this in ClientCallbackHandler using the System.console().

String... args) {
try {
Process process = startProcess(directory, command, args);
new HandleOutput(process.getInputStream(), log).run();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're not launching this in a new thread? It seems like we should to ensure we get all the content.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why we would need to.
The Process is started on its own thread and we are redirecting its input to log it in the main thread.
We are then waiting for the process to finish up. We should have all the content displayed when the process is finished

Copy link
Member

Choose a reason for hiding this comment

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

I definitely could be wrong, but won't it block until the process is complete? I guess this one is done without a timeout so maybe not a big deal and the next process.waitFor() would block anyway. It probably doesn't matter, just looked strange to me I suppose :)

Copy link
Member Author

@jmesnil jmesnil Sep 20, 2022

Choose a reason for hiding this comment

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

yes, this one will block until the process is complete.
There is no timeout on this one (eg building the docker image can take as many time as it needs)

As an user, I want to build and push an application image from the
plugin

JIRA: https://issues.redhat.com/browse/WFMP-174

Signed-off-by: Jeff Mesnil <jmesnil@redhat.com>
@jamezp jamezp merged commit 623671f into wildfly:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants