Fixes #8409 - Pull docker image asynchronously #72
Conversation
| @@ -73,23 +69,19 @@ def provider_friendly_name | |||
| end | |||
|
|
|||
| def create_container(args = {}) | |||
| options = vm_instance_defaults.merge(args) | |||
| options = vm_instance_defaults.merge(args) { |_, default, new| new.empty? ? default : new } | |||
There was a problem hiding this comment.
This was changed so that when no Cmd is specified, it uses the default. Otherwise a .merge will not replace a [] as it's not nil.
dLobatog
added
Waiting on contributor
and removed
Not yet reviewed
Needs testing
labels
| container | ||
| end | ||
|
|
||
| # destroy_wizard_state(wizard_state) |
There was a problem hiding this comment.
I left this commented out while I was developing this feature - should NOT be commented normally
dLobatog
force-pushed
the
8409
branch
2 times, most recently
from
5e65e19
to
7989dee
Compare
|
Updated to let the action be a good thread citizen and not block the pool. To be tested with foreman core develop, I'll submit a patch with 1.7 compat fixes separately so we can keep track of these. cc @witlessbird @daviddavis @parthaa can you test it? |
| rescue => e | ||
| action_logger.error(e.message) | ||
| action_logger.debug(e.backtrace.join("\n ")) | ||
| error!(e) |
There was a problem hiding this comment.
This is not a thread save call, changing the state of the action from a different thread. The error needs to be sent back to the action same as :done event.
dLobatog
force-pushed
the
8409
branch
2 times, most recently
from
f87c0eb
to
699f828
Compare
dLobatog
force-pushed
the
8409
branch
4 times, most recently
from
e3f5f03
to
366642a
Compare
|
re the test failure, what appears to be happening is that foreman-tasks is registering its extensions to the test rake task from its engine.rb initialisers. foreman-docker is of course requiring foreman-tasks, but the 'test' task(s) are now extended with both foreman-docker's and foreman-tasks' tests. I don't know whether we should be running foreman-tasks' tests from here or not. We run core tests from plugins to check a plugin didn't break a core behaviour, and you could argue whether that should be the case or not for a plugin building on another plugin. It might be harder, since plugin tests may require additional development dependencies. If you do want to run foreman-tasks tests, then I think something's wrong with the load paths (here?). If not, then some refactoring of how rake tasks are modified (e.g. so they only happen when testing the plugin itself) or how we choose which sets are run is needed.. I need to think a bit more about that I guess. |
| if started | ||
| container.update_attribute(:uuid, started.id) | ||
| else | ||
| errors << container.compute_resource.errors[:base] |
There was a problem hiding this comment.
This is lacking a test - and errors is not defined
|
This PR should address the issue with the foremant-tasks tests (so that the tasks tests should not be run as part of this engine tests) theforeman/foreman-tasks#103 |
|
@iNecas Awesome! Any prospective dates for a release? |
|
Hopefully today, we have other feature that I would like to get in before the release, but if we don't get that done till tomorrow, i will release new version without it anyway. |
|
The new versions in a PR for foreman-packaging theforeman/foreman-packaging#582 |
|
[test] |
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
|
[test] |
|
[test] this got aborted I think |
dLobatog
force-pushed
the
8409
branch
2 times, most recently
from
efe3a69
to
907e59d
Compare
|
[test] - tests were failing because I was using minitest-reporters here (and it's not used in Foreman core) |
|
@daviddavis @parthaa @witlessbird Could you give a last test at this before merging? |
|
looks good to me. |
|
I've been testing this a bit more thoroughly myself and I think this should:
I'll polish it up with these changes & resubmit when I get a chance. |
daviddavis
added
Waiting on contributor
and removed
Needs re-review
Needs testing
labels
|
@dLobatog any updates? |
|
I'll pick this up again when ActiveJob is available |
No description provided.