Oups, PR conflict here !

YunoHost-Apps / searx_ynh

Searx package for YunoHost

https://searx.github.io/searx/

GNU General Public License v3.0

20 stars 18 forks source link

Oups, PR conflict here ! #14

Closed maniackcrudelis closed 7 years ago

maniackcrudelis commented 7 years ago

2 PR at the same time, and we have duplicated this include. To works fine, this include need to be at the end of the file.

[x] Complete test
[ ] Upgrade previous version
[x] Code review
[x] Approval (LGTM) JimboJoe
[x] Approval (LGTM)
[x] CI succeeded

Minor decision, close on 13 of july, or 9 of july.

frju365 commented 7 years ago

For me the PR doesn't need a complete test. Upgrade is perhaps enough for the long tests category. For me :

[X] Code review
[X] Approval (LGTM)

maniackcrudelis commented 7 years ago

I thought at a test of nginx. Try an install and check if nginx still works fine and if the panel appears.

frju365 commented 7 years ago

[X] Complete test Everything is ok and the panel appears (logo at the bottom + panel on click)

maniackcrudelis commented 7 years ago

Nice ;)

I would like to perform a pcheck test. But, currently this new method doesn't allow the dev who make the PR to check the boxes. I think we should think about that...

frju365 commented 7 years ago

CI succeded is ok, no ?

JimboJoe commented 7 years ago

LGTM 👍

maniackcrudelis commented 7 years ago

CI succeded is ok, no ?

No, our CI doesn't check the pull request. So we have to perform this test on our own.

For what it's worth, I did a test. https://paste.yunohost.org/idojidubif

frju365 commented 7 years ago

[X] CI Succeded test is ok, I think. I already have verified the test you did.

JimboJoe commented 7 years ago

@maniackcrudelis If you're OK, we could merge that, couldn't we?

maniackcrudelis commented 7 years ago

Yes we can, the upgrade test is not needed. We know that the nginx config will be rewrite by the upgrade.