Skip to content
This repository was archived by the owner on Mar 16, 2021. It is now read-only.

Fixes #8409 - Pull docker image asynchronously#72

Open
dLobatog wants to merge 1 commit into
theforeman:masterfrom
dLobatog:8409
Open

Fixes #8409 - Pull docker image asynchronously#72
dLobatog wants to merge 1 commit into
theforeman:masterfrom
dLobatog:8409

Conversation

@dLobatog

Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I left this commented out while I was developing this feature - should NOT be commented normally

@dLobatog

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
dLobatog force-pushed the 8409 branch 2 times, most recently from f87c0eb to 699f828 Compare January 29, 2015 14:09
@dLobatog

Copy link
Copy Markdown
Member Author

Updated with a test for the provision plan and commented out the tests for the run phase of start and pull. I'll work with @iNecas or @pitr-ch trying to figure out what I'm doing wrong so that these tests don't pass.

@dLobatog
dLobatog force-pushed the 8409 branch 4 times, most recently from e3f5f03 to 366642a Compare February 3, 2015 01:38
@domcleal

domcleal commented Feb 3, 2015

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is lacking a test - and errors is not defined

@iNecas

iNecas commented Mar 13, 2015

Copy link
Copy Markdown
Member

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

@dLobatog

Copy link
Copy Markdown
Member Author

@iNecas Awesome! Any prospective dates for a release?

@iNecas

iNecas commented Mar 16, 2015

Copy link
Copy Markdown
Member

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.

@iNecas

iNecas commented Mar 17, 2015

Copy link
Copy Markdown
Member

The new versions in a PR for foreman-packaging theforeman/foreman-packaging#582

@iNecas

iNecas commented Mar 17, 2015

Copy link
Copy Markdown
Member

[test]

@dLobatog

Copy link
Copy Markdown
Member Author

[test]

@dLobatog

Copy link
Copy Markdown
Member Author

[test] this got aborted I think

@dLobatog
dLobatog force-pushed the 8409 branch 2 times, most recently from efe3a69 to 907e59d Compare April 7, 2015 11:33
@dLobatog

dLobatog commented Apr 8, 2015

Copy link
Copy Markdown
Member Author

[test] - tests were failing because I was using minitest-reporters here (and it's not used in Foreman core)

@dLobatog

dLobatog commented Apr 8, 2015

Copy link
Copy Markdown
Member Author

@daviddavis @parthaa @witlessbird Could you give a last test at this before merging?

@dmitri-d

dmitri-d commented Apr 9, 2015

Copy link
Copy Markdown
Member

looks good to me.

@dLobatog

Copy link
Copy Markdown
Member Author

I've been testing this a bit more thoroughly myself and I think this should:

  • Show a link to the task created when creating a container, telling the user to check Monitor > Tasks is not very user friendly.
  • If the task fails, show in the show container page that the task in question failed

I'll polish it up with these changes & resubmit when I get a chance.

@daviddavis

Copy link
Copy Markdown

@dLobatog any updates?

@dLobatog

dLobatog commented Mar 9, 2017

Copy link
Copy Markdown
Member Author

I'll pick this up again when ActiveJob is available

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants