search

YunoHost-Apps / Experimental_helpers

6 stars 12 forks source link

pgsql: Change the way we run commands fix #13 and improve documentation #14

Closed Jibec closed 6 years ago

JimboJoe commented 6 years ago

So why the --login fixes the original issue? Tested OK on my side!

Jibec commented 6 years ago

Because what is does is a real login with the user, so you'll be in user's folder, instead of running in the current folder.

As it also makes it easier to read, I decided to rewrite everything I could using the same method. It also triggers errors (like already existing role, which I fixed here :))

alexAubin commented 6 years ago

Any feedback on this guys ? Shall we integrate this in https://github.com/YunoHost/yunohost/pull/238 to integrate it in the core ? I don't know how this relates to https://github.com/YunoHost-Apps/Experimental_helpers/pull/15 ...

JimboJoe commented 6 years ago

Well, #15 only brings the ynh_exec_login_as to use instead of sudo --login -u but that can easily be postponed. Otherwise these helpers seem mature enough for me to integrate into https://github.com/YunoHost/yunohost/pull/238 :+1:

alexAubin commented 6 years ago

PIng @Jibec : shall we merge this as it is, or shall we integrate #15 ?

Jibec commented 6 years ago

I don't mind, but I won't do it, I don't understand the benefit

alexAubin commented 6 years ago

That's to avoid having to prefix everything with sudo --login --user=postgres ... of course then you have to prefix with exec_login_as so that's debatable :P

Anyway I think it's simpler to not integrate #15 since that creates a dependency to another non-mandatory helper which would again delay the integration into the core ... if exec_login_as really has added value in this context, we can discuss / change the helper later if needed I think.

So let's say we merge this and propose to integrate this in the core ?

JimboJoe commented 6 years ago

Agreed! :+1:

alexAubin commented 6 years ago

@Jibec Do you agree ? :stuck_out_tongue:

(btw I don't have merging rights here, I'm just trying to make this thing move forward :P )

Edit : actually it looks like @Jibec already implicitely agreed in his previous comment so maybe we can merge right away

Jibec commented 6 years ago

indeed, I agreed

alexAubin commented 6 years ago

So it looks like we have 4 persons agreeing. Can someone click on merge ? :D