all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Gabriel Goller <g.goller@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup v4 2/2] close #4723: api: added endpoint for gc status
Date: Wed, 27 Sep 2023 17:19:42 +0200	[thread overview]
Message-ID: <7cfcd592-4a5b-4b94-832f-22c0397768bb@proxmox.com> (raw)
In-Reply-To: <20230927130608.166028-2-g.goller@proxmox.com>

Am 27/09/2023 um 15:06 schrieb Gabriel Goller:
> update v4:
>  - separate commits for ui/api

No thorough review, I'm afraid, but two things I noticed when skimming
this:

1. Please order patches such that the dependency order is correct.
Here you added the UI using a new API endpoint before adding said
endpoint.

It's easier to review, and less error-prone, e.g., if there are patches
that seem like they could be applied already, while some other parts of
such a patch series might still need rework.

I hope I (or maybe someone else) can give this a more thorough review.
If the order and the endpoint name (see below) will stay the only two
issues, there's no hard need for a v3, I can also fix those up on
applying.

>  - cleaned up code
>  - show 'next scheduled run', when no gc has ever been run


> @@ -2265,6 +2383,10 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>              .get(&API_METHOD_GARBAGE_COLLECTION_STATUS)
>              .post(&API_METHOD_START_GARBAGE_COLLECTION),
>      ),
> +    (
> +        "gc_info",

2. Please use kepbab-case, not snake_case, for things part of the public
API, like the URL here (but also parameters, which you already handle
correctly).




      reply	other threads:[~2023-09-27 15:20 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-27 13:06 [pbs-devel] [PATCH proxmox-backup v4 1/2] close #4723: ui: new gc view Gabriel Goller
2023-09-27 13:06 ` [pbs-devel] [PATCH proxmox-backup v4 2/2] close #4723: api: added endpoint for gc status Gabriel Goller
2023-09-27 15:19   ` Thomas Lamprecht [this message]

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=7cfcd592-4a5b-4b94-832f-22c0397768bb@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=g.goller@proxmox.com \
    --cc=pbs-devel@lists.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal