Re: git based code review
On 2024-06-24 16:18, Ekaitz Zarraga wrote: > Hi, > > On 2024-06-24 11:31, raingl...@riseup.net wrote: >> Since the email based workflow has put off quite a few potential >> contributors, maybe this could be a good distributed alternative: >> https://github.com/google/git-appraise >> >> Disclaimer: haven't tried it myself, also haven't tried packaging it. >> The Go and Rust dependencies could be hard to package with Guix's >> current tools. >> > > Arun is also developing a similar thing but it's not based on the git-notes > for the issue system. I don't remember the name, maybe he can talk more about > it. > > Also `fossil` does all this but "better" integrated. > > It's a very interesting thing to test but also our repo is already kind of > huge. If we merge all the issues and stuff in it... It might become even > gigantic. > > Also, the Mumi based workflow described here: > https://guix.gnu.org/manual/devel/en/html_node/Debbugs-User-Interfaces.html#Command_002dline-interface > > ...is probably way better than using `git send-email` raw. WDYT? > > I started with `git send-email` not that long ago, and *I* didn't find the > workflow hard to follow. In fact, *I* think it's even easier than the PR > based thing that is widespread right now. > > I'm not saying it isn't true that there is people that was put off by the > email based workflow, but I'd like to know how many. > > In any case, it's a very interesting link! Thanks for sharing! > > Cheers! This is specifically for code *review*, not issue tracking or simply sending patches. PR review was one of the major problems with the email tools that I've seen mentioned.
Re: git based code review
Hi, On 2024-06-24 11:31, raingl...@riseup.net wrote: Since the email based workflow has put off quite a few potential contributors, maybe this could be a good distributed alternative: https://github.com/google/git-appraise Disclaimer: haven't tried it myself, also haven't tried packaging it. The Go and Rust dependencies could be hard to package with Guix's current tools. Arun is also developing a similar thing but it's not based on the git-notes for the issue system. I don't remember the name, maybe he can talk more about it. Also `fossil` does all this but "better" integrated. It's a very interesting thing to test but also our repo is already kind of huge. If we merge all the issues and stuff in it... It might become even gigantic. Also, the Mumi based workflow described here: https://guix.gnu.org/manual/devel/en/html_node/Debbugs-User-Interfaces.html#Command_002dline-interface ...is probably way better than using `git send-email` raw. WDYT? I started with `git send-email` not that long ago, and *I* didn't find the workflow hard to follow. In fact, *I* think it's even easier than the PR based thing that is widespread right now. I'm not saying it isn't true that there is people that was put off by the email based workflow, but I'd like to know how many. In any case, it's a very interesting link! Thanks for sharing! Cheers!
git based code review
Since the email based workflow has put off quite a few potential contributors, maybe this could be a good distributed alternative: https://github.com/google/git-appraise Disclaimer: haven't tried it myself, also haven't tried packaging it. The Go and Rust dependencies could be hard to package with Guix's current tools.
Re: code review
2017-09-12 21:35 GMT+02:00 Christopher Baines : > On Mon, 11 Sep 2017 22:10:22 +0200 > Catonano wrote: > > > I could use some code review on my Trytond service hypothesis > > > > As far as I understand, there are 2 steps that need to be done in > > order for a Trytond service to be usable. > > > > 1) as the "postgres" role (that is as the operating system "postgres" > > user), create a "tryton" role > > > > 2) as the tryton user (hence under the tryton role) run the Tryton > > initialization script (trytond-admin -c -d > name> --all) > > > > I feel like I should extend the postgresql service in order to create > > te trytond role but I don't know how > > I don't think there is any current approach for doing this. > > Service extension could be one way of doing it. It's similar to how > other services in Guix interact with each other. > > However, I believe creating a role can only be done when the PostgreSQL > server is running, which means that you'd need to defer creating the > role until that point. Keeping this in a single shepherd service might > not be easy to implement, and even if you did, there are some usability > issues, e.g. if you add a new service to the system, and that service > also extends PostgreSQL, then you might run in to trouble creating the > role for the new service, without needlessly restarting the PostgreSQL > service in the process. > > One approach I've implemented and used [1] is to do create roles for > PostgreSQL when you start the service that uses that role. > Yes, I'll work on this approach from now on > > I also remember Ludo suggesting using additional services to handle > this kind of setup, which I understood to be something like a > tryton-postgresql-role shepherd service, which the tryton shepherd > service would require. > I attempted this but my trytond-postgres-role-service-type doesn't extend shepherd-root-service-type I chose not to extend shepherd-root-service-type because the trytond role doesn't require a daemon running It has to be created one off (una tantum) so, I tought, a simple activation will do But now, the trytond-service-type (in charge of running the trytond daemon) should require trytond-postgres-role among its dependencies but there is NO sheperd service provisioning a postgres role So this is what I end up with (when testing the system with the marionette machinery) srfi/srfi-1.scm:640:9: In procedure for-each: srfi/srfi-1.scm:640:9: Throw to key `srfi-34' with args `(#)'. make: *** [Makefile:5295: check-system] Error 1 Should I make the trytond-postgres-role a sheperd service ? Admittedly I don't like the idea So, I'm thinking, I'll get back to creating the postgres role upon activation of the trytond-service-type I failed to foresee this issue so I wasted some work :-/ The branch is here, should anyone want to take a look (pay attention, I rebase carelessly) https://gitlab.com/humanitiesNerd/guix-hacks
Re: code review
2017-09-12 21:35 GMT+02:00 Christopher Baines : > On Mon, 11 Sep 2017 22:10:22 +0200 > Catonano wrote: > Hi Chris, thank you for your review. And I apologize for the long delay. I tried to follow some of your suggestions here https://gitlab.com/humanitiesNerd/guix-hacks/blob/trytonservice/gnu/services/trytond.scm I didn't try to implement the abstraction for the postgres connections The postgres service right now uses the operating system user "postgres" in order to connect under the "postgres" role, so avoiding to use a password I did the same with the "tryton" operating system user and the "tryton" role This assumes that the postgres service and thhe Tryton service are rnning on the same server and this could be restrictive in some situations Honestly I wasn't willing to wrap my mind around I just want to have a minimum Tryton service running Postgres commands for connecting safely to a remote postgres server I just want to have a minimum Tryton service running I hope this is ok Anyway, now I should try to write some system tests or these 2 services I wrote Any patch would be welcome ;-) Thanks
Re: code review
On Mon, 11 Sep 2017 22:10:22 +0200 Catonano wrote: > I could use some code review on my Trytond service hypothesis > > As far as I understand, there are 2 steps that need to be done in > order for a Trytond service to be usable. > > 1) as the "postgres" role (that is as the operating system "postgres" > user), create a "tryton" role > > 2) as the tryton user (hence under the tryton role) run the Tryton > initialization script (trytond-admin -c -d name> --all) > > I feel like I should extend the postgresql service in order to create > te trytond role but I don't know how I don't think there is any current approach for doing this. Service extension could be one way of doing it. It's similar to how other services in Guix interact with each other. However, I believe creating a role can only be done when the PostgreSQL server is running, which means that you'd need to defer creating the role until that point. Keeping this in a single shepherd service might not be easy to implement, and even if you did, there are some usability issues, e.g. if you add a new service to the system, and that service also extends PostgreSQL, then you might run in to trouble creating the role for the new service, without needlessly restarting the PostgreSQL service in the process. One approach I've implemented and used [1] is to do create roles for PostgreSQL when you start the service that uses that role. I also remember Ludo suggesting using additional services to handle this kind of setup, which I understood to be something like a tryton-postgresql-role shepherd service, which the tryton shepherd service would require. 1: https://github.com/alphagov/govuk-guix > Also, I create the trytond role with no password. But what if someone > wants to use this service for a real server ? > > The role password should be a parameter somehow. Again, I'm not sure > how It looks to be configurable at the moment, as someone using the service can pass through there own value for the config-file field in the . On the subject of database connections, again, when trying to make something work with the govuk-guix project, I ended up making record types for different database connections (e.g. [2]). This makes it easy to work with as data, and then convert to a URI when you need one. I'd be interested in seeing this in Guix, as I think it would be really useful when trying to write services that use databases. 2: https://github.com/alphagov/govuk-guix/blob/master/gds/services/utils/databases/postgresql.scm#L24 > I borrowed some code from the postgresql service code and somewhat > edited it > > But I feel like an amateur neurosurgeon :-/ > > I could really use a review of this code > > Here it is > > (define-module (gnu services trytond) > #:use-module (gnu services) > #:use-module (gnu services shepherd) > #:use-module (gnu system shadow) > #:use-module (gnu packages admin) > ;; do I really need to access the postgresql package, here ? > #:use-module (gnu packages databases) > #:use-module (gnu packages tryton) > #:use-module (guix modules) > #:use-module (guix records) > #:use-module (guix gexp) > #:use-module (ice-9 match) > #:export (trytond-configuration > trytond-configuration? > trytond-service > trytond-service-type > )) > > ;;; Commentary: > ;;; > ;;; Trytond based services. Mainly Trytond and GNUHealth for now > ;;; > ;;; Code: > > (define-record-type* > trytond-configuration make-trytond-configuration > trytond-configuration? > (trytond trytond-configuration-trytond ; >(default trytond)) > ;; do I really need to access the postgresql package, here ? > (postgresql postgresql-configuration-trytond >(default postgresql)) > (locale trytond-configuration-locale > (default "en_US.utf8")) > (config-filetrytond-configuration-file) > (data-directory trytond-configuration-data-directory) > ) > > > (define %default-trytond-config > (mixed-text-file "trytond.conf" >"[database]\n" >;; how do I connect with a role that has no > password ? ;; I create the trytond role without the password >;; but what if someone wants to use this service > for a real server ? >;; the password should be a parameter, somehow >;;"uri = 'postgresql://trytond:password@/'\n" >"uri = 'postgresql://trytond:@/'\n" ;; is this > string gonna work ? >"path = /var/lib/trytond"))
code review
I could use some code review on my Trytond service hypothesis As far as I understand, there are 2 steps that need to be done in order for a Trytond service to be usable. 1) as the "postgres" role (that is as the operating system "postgres" user), create a "tryton" role 2) as the tryton user (hence under the tryton role) run the Tryton initialization script (trytond-admin -c -d --all) I feel like I should extend the postgresql service in order to create te trytond role but I don't know how Also, I create the trytond role with no password. But what if someone wants to use this service for a real server ? The role password should be a parameter somehow. Again, I'm not sure how I borrowed some code from the postgresql service code and somewhat edited it But I feel like an amateur neurosurgeon :-/ I could really use a review of this code Here it is (define-module (gnu services trytond) #:use-module (gnu services) #:use-module (gnu services shepherd) #:use-module (gnu system shadow) #:use-module (gnu packages admin) ;; do I really need to access the postgresql package, here ? #:use-module (gnu packages databases) #:use-module (gnu packages tryton) #:use-module (guix modules) #:use-module (guix records) #:use-module (guix gexp) #:use-module (ice-9 match) #:export (trytond-configuration trytond-configuration? trytond-service trytond-service-type )) ;;; Commentary: ;;; ;;; Trytond based services. Mainly Trytond and GNUHealth for now ;;; ;;; Code: (define-record-type* trytond-configuration make-trytond-configuration trytond-configuration? (trytond trytond-configuration-trytond ; (default trytond)) ;; do I really need to access the postgresql package, here ? (postgresql postgresql-configuration-trytond (default postgresql)) (locale trytond-configuration-locale (default "en_US.utf8")) (config-filetrytond-configuration-file) (data-directory trytond-configuration-data-directory) ) (define %default-trytond-config (mixed-text-file "trytond.conf" "[database]\n" ;; how do I connect with a role that has no password ? ;; I create the trytond role without the password ;; but what if someone wants to use this service for a real server ? ;; the password should be a parameter, somehow ;;"uri = 'postgresql://trytond:password@/'\n" "uri = 'postgresql://trytond:@/'\n" ;; is this string gonna work ? "path = /var/lib/trytond")) (define %trytond-accounts (list (user-group (name "trytond") (system? #t)) (user-account (name "trytond") (group "trytond") (system? #t) (comment "Trytond server user") (home-directory "/var/empty") (shell (file-append shadow "/sbin/nologin") (define trytond-activation (match-lambda (($ trytond postgresql locale config-file data-directory) #~(begin (use-modules (guix build utils) (gnu packages database) (ice-9 match)) (let ((trytond-user (getpwnam "trytond")) (postgres-user (getpwnam "postgres")) (create-the-trytond-role (string-append #$postgresql "/bin/createuser" "trytond" "-d")) ;; the role can create new DBs (run-the-trytond-init-script (string-append #$trytond "/bin/trytond-admin" "-c" #$config-file "-d" "trytondb" ;;the database name "--all")) (trytond-initscript-args (append (if #$locale (list (string-append "-l " #$locale)) '() ;; Create data directory. (mkdir-p #$data-directory) (chown #$data-directory (passwd:uid trytond-user) (passwd:gid trytond-user)) ;; Drop privileges and create the tryton role in a new ;; process. Wait for it to finish before proceeding. ;; shouldn't this be done by extending the postgresql service ? ;; but how ? (match (primitive-fork) (0 ;; Exit with a non-
