#393 modify the code to valid docker deployment
Closed by lnie. Opened by lnie.
taskotron/ lnie/libtaskotron feature-docker  into  feature/ansiblize

Download 393.patch

I have modified roshi's docker code:)
Just want to have a try and lots of follow-up work need to be done,that's for sure.
test plan:
git clone https://pagure.io/task-rpmlint-ansible
runtask -d -i gzip-1.8-1.fc25 -t koji_build --docker -a x86_64 task-rpmlint-ansible/run_tests.yml
or git clone https://github.com/stefwalter/gzip-dist-git
runtask -d -i gzip -t koji_build -a x86_64 --docker gzip-dist-git/tests/test_rpm.yml

Hi Lili. Are you saying this is a modification of PR #388? What exactly changed? Have you only rebased it against current feature/ansiblize or performed further modifications?

Also, please note that I'd like to merge feature/ansiblize into develop first, that is our current top priority. Then we can deal with docker.

Hi Lili. Are you saying this is a modification of PR #388? What exactly changed? Have you only >rebased it against current feature/ansiblize or performed further modifications?

I have rebased it against current 'feature/ansiblize' and modified some code according to the comments from https://phab.qa.fedoraproject.org/D1196,
as it seems that roshi doesn't has enough time to do the modification.
I also have fixed some potential exceptions,such as port bind errors when taskotron tries to use an occupied port,though I just found my resolution is not strong enough,
and will update the code when it's asked.

Also, please note that I'd like to merge feature/ansiblize into develop first, that is our >current top priority. Then we can deal with docker.

Thanks for making it clear,don't want to be so reckless,just get some time and try to do a help as it seems that there is some confusion about docker during our last team meeting,
and our team members are busy with all kinds of other important things.I also have found a few other errors(just now) in feature/anisblize itself( fixed one in this pull request),
should I fix it now,or wait for a period of time ?

Thanks for explanation, and the effort. Let's make feature/ansiblize working first, and then we'll look at this. If you see any issues with feature/ansiblize, feel free to submit PRs against it. It's better to submit multiple smaller PRs, each fixing one particular problem, than a single big PR changing too many things at once. Also, if you want recommendations what to work on (not necessarily libtaskotron), I'm sure we can come up with something.

If this is "just" rebase + fixes of Mike's patch, I'd like to see it split into (at least) two commits - one which handles the rebasing, and the second that applies the changes, to preserve Mike's authorship/contribuiton as much as possible.

Thanks for explanation, and the effort. Let's make feature/ansiblize working first, and then we'll >look at this. If you see any issues with feature/ansiblize, feel free to submit PRs against it. It's >better to submit multiple smaller PRs, each fixing one particular problem, than a single big PR >changing too many things at once. Also, if you want recommendations what to work on (not >necessarily libtaskotron), I'm sure we can come up with something.

Sure thing, and gonna to submit smaller ones ,though just little modifications.

If this is "just" rebase + fixes of Mike's patch, I'd like to see it split into (at least) two commits - >one which handles the rebasing, and the second that applies the changes, to preserve Mike's >authorship/contribuiton as much as possible.

That's really make the point, this is my first pull request,I don't really clear about how it works,as you know,I even asked you whether there is a way to get the code reviewed yesterday.Thinking it will cause conflicts if I make a new pr on top of Mike's,I'm hesitating about whether I should make the new pr while learning how to make pr.But I finally click the button,as I 'm 100% sure this won't be merged,one reason is it need further modification,the second is I want to make a pr.

Anyway, disregarding the formal issues, this also really needs tests in order to be merged. Please have a look at how our code is tested (we use py.test, and all the tests are located in the /testing directory in the git root), and add reasonable tests covering the added functionality. We can make sure the code is rightfully appropriated at merge-time.

Anyway, disregarding the formal issues, this also really needs tests in order to be merged. >Please have a look at how our code is tested (we use py.test, and all the tests are located in the >/testing directory in the git root), and add reasonable tests covering the added functionality. We >can make sure the code is rightfully appropriated at merge-time.

Got it,and gonna to do.

Pull-Request has been closed by lnie

This has been replaced by #400.

Metadata