Hw15/enavarro #14
Hw15/enavarro #14
Conversation
src/lib/ydocker/main_dialog.rb
Outdated
| return unless (Yast::Popup.YesNo(_("Do you want to remove the container?"))) | ||
| selected_container.delete | ||
| def start_container | ||
| return unless (Yast::Popup.YesNo(_("Do you really want to start the container?"))) |
There was a problem hiding this comment.
is this question needed? I think start is non-problematic operation, as you can easily undo it by kiling it ( which is opposite to stop/kill which is harder to undo and can cause operation corruption
src/lib/ydocker/main_dialog.rb
Outdated
| selected_container.stop! | ||
| redraw_containers | ||
| end | ||
| if Yast::Popup.YesNo(_("Do you want to remove the container?")) |
There was a problem hiding this comment.
does it make sense if answers are "no" and "yes"? it means that you do not stop container but remove it
src/lib/ydocker/main_dialog.rb
Outdated
| selected_container.kill! | ||
| redraw_containers | ||
| end | ||
| if Yast::Popup.YesNo(_("Do you want to remove the container?")) |
src/lib/ydocker/main_dialog.rb
Outdated
| Yast::UI.ChangeWidget(:container_start, :Enabled, status != 'running') | ||
| Yast::UI.ChangeWidget(:container_changes, :Enabled, status == 'running') | ||
| Yast::UI.ChangeWidget(:container_inject, :Enabled, status == 'running') | ||
| Yast::UI.ChangeWidget(:container_stop, :Enabled, status == 'running') |
There was a problem hiding this comment.
a bit duplicite, maybe use instead running = selected_container.info['State']['Status'] == "Running" and then use running or !running as all of them check same status.
| require "yast" | ||
|
|
||
| module YDocker | ||
| class PullImageDialog |
There was a problem hiding this comment.
JFYI after writting docker we found useful base class for this kind of dialogs, that make code a bit smaller and nicer, so you can use it next time 😉
http://www.rubydoc.info/github/yast/yast-yast2/UI/Dialog
src/lib/ydocker/pull_image_dialog.rb
Outdated
| class PullImageDialog | ||
| include Yast::UIShortcuts | ||
| include Yast::I18n | ||
| extend Yast::I18n |
There was a problem hiding this comment.
why extend is needed? it should not be necessary as all translations are done in instance methods.
There was a problem hiding this comment.
why extend is needed? it should not be necessary as all translations are done in instance methods.
Deleted.
src/lib/ydocker/pull_image_dialog.rb
Outdated
| include Yast::I18n | ||
| extend Yast::I18n | ||
|
|
||
| def initialize() |
There was a problem hiding this comment.
parens not needed. Maybe time to add rubocop even to yast2-docker? we already use it in some modules and also have shared config with yast unification - see e.g. https://github.com/yast/yast-bootloader/blob/master/.rubocop.yml#L3
There was a problem hiding this comment.
parens not needed. Maybe time to add rubocop even to yast2-docker? we already use it in some modules and also have shared config with yast unification - see e.g. https://github.com/yast/yast-bootloader/blob/master/.rubocop.yml#L3
Deleted. I will try to do a pull request to include rubocop.
| Opt(:notify), | ||
| _("Source") | ||
| ) | ||
| ) |
There was a problem hiding this comment.
what exactly is this source? if it is path on system, it is possible to have search button there, but if it is some network location. it can be a more tricky.
There was a problem hiding this comment.
Hi :)
It isn't a path on the system. The source is the name of the docker image in docker hub. Also, source is the parameter NAME:TAG in 'docker pull NAME:TAG'.
|
in general it looks good, just few notes. |
|
@eduardoj will try to finish it |
eduardoj
force-pushed
the
HW15/enavarro
branch
from
147543d
to
978a72b
Compare
|
@eduardoj, don't forget to fix the default command of the worker when running a container with multi-parameter in exec-form. |
eduardoj
force-pushed
the
HW15/enavarro
branch
from
978a72b
to
ff4d84f
Compare
- Obtain the correct command parameter whether: - it is not defined - it is a string - or, it is an array
Change table headers. Add column port to Ports table.
Run command option is now left aligned.
eduardoj
force-pushed
the
HW15/enavarro
branch
from
ff4d84f
to
b1a1492
Compare
|
@eduardoj @SergioAtSUSE the PR seems to look good, but it still needs a changelog entry with a bug number or fate associated with it and also a version increase for being approved, could you check it? |
|
@eduardoj @SergioAtSUSE will close it by now as it has not received any attention during the last year so if you plan to add missing changelog and bump version feel free to reopen it. |
Improve yast-docker module for basic docker operations. With this changes you can:
This was made during hackweek 15 to perform graphic installation of openQA.
Pair programming with @SergioAtSUSE