From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Dominik Csapak <d.csapak@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH access-control 1/1] add realmsync plugin for jobs and CRUD api for domainsync-jobs
Date: Tue, 8 Nov 2022 11:36:03 +0100 [thread overview]
Message-ID: <20221108103603.qh4opljilbnek4ua@casey.proxmox.com> (raw)
In-Reply-To: <0029248b-d216-edcb-51f5-d5157ab871dd@proxmox.com>
On Tue, Nov 08, 2022 at 10:24:50AM +0100, Thomas Lamprecht wrote:
> Am 08/11/2022 um 09:20 schrieb Dominik Csapak:
> >> is this outdated due to being from april? or do we really share the
> >> ID namespace between all plugin types?
> >
> > thats normal for section configs, but usually we don't notice in pve
> > since we normally only have a single endpoint for listing all objects
> > and not per 'type'
> > (e.g. you can't have 2 storages with the same ids but different types
> > either)
>
> @wolfgang didn't you improve on that somewhere by some xyz-id prefix or the
> like?
I don't think so, not for the jobs api anyway. There may have been
off-list discussions?
If we really want to keep a single `jobs.cfg` for multiple types, maybe
we should also forbid accessing the jobs directly via
`$jobs->{ids}->{$id}` and require going over accessors that require a
type parameter.
(Maybe bless the $jobs hash into a dedicated package for convenience, so
we can do $jobs->get($id, $type).)
>
> >>> + foreach my $k (keys %$param) {
> >>
> >> probably just copy "error", but please: s/foreach/for/, or even:
> >>
> >> $job->{$_} = $param->{$_} for keys $param->%*;
> >
> > mhmm.. AFAIR i did not see that pattern anywhere yet in our codebase, maybe we want
> > an example of that in our style guide? (for single line loops i like it)
The style guide actually already specifies the preference of `for` over
`foreach` ;-)
next prev parent reply other threads:[~2022-11-08 10:36 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-04 8:54 [pve-devel] [PATCH access-control/manager] add realm sync jobs Dominik Csapak
2022-04-04 8:54 ` [pve-devel] [PATCH access-control 1/1] add realmsync plugin for jobs and CRUD api for domainsync-jobs Dominik Csapak
2022-11-07 17:05 ` Thomas Lamprecht
2022-11-08 8:20 ` Dominik Csapak
2022-11-08 9:24 ` Thomas Lamprecht
2022-11-08 10:36 ` Wolfgang Bumiller [this message]
2022-04-04 8:54 ` [pve-devel] [PATCH manager 1/4] Jobs: provide id and schedule to the job Dominik Csapak
2022-11-07 16:14 ` [pve-devel] applied: " Thomas Lamprecht
2022-04-04 8:54 ` [pve-devel] [PATCH manager 2/4] Jobs/Plugin: remove 'vzdump' from id description Dominik Csapak
2022-11-07 16:14 ` [pve-devel] applied: " Thomas Lamprecht
2022-04-04 8:54 ` [pve-devel] [PATCH manager 3/4] Jobs: add RealmSync Plugin and register it Dominik Csapak
2022-11-07 16:14 ` [pve-devel] applied: " Thomas Lamprecht
2022-11-07 16:15 ` Thomas Lamprecht
2022-04-04 8:54 ` [pve-devel] [PATCH manager 4/4] ui: add Realm Sync panel Dominik Csapak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221108103603.qh4opljilbnek4ua@casey.proxmox.com \
--to=w.bumiller@proxmox.com \
--cc=d.csapak@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox