"month" arguments '08' and '09' are broken

[davidgubler]

Description

The appuio-odoo-adapter does not work with month arguments '08' and '09'.

This is because the 'cli' package's "IntFlag" does not use base 10 by default. Instead it auto-detects the base. In case of strings starting with '0', it assumes base 8, hence '08' and '09' are not parseable.

The culprit is here: https://github.com/urfave/cli/blob/main/flag_int.go#L47 This code should use base 10 by default instead of 0 (auto-detect).

Unfortunately this is not configurable, so I don't see an easy fix, but maybe I'm overlooking something. Personally I would completely remove the 'cli' library and do some simple environment variable parsing instead, but I presume some people will disagree with me on that.

Additional Context

I will try to find a workaround by changing the CronJob such that it supplies the month without leading zero, see https://github.com/appuio/component-appuio-cloud-reporting/blob/master/component/main.jsonnet#L195

Logs

Incorrect Usage: invalid value "08" for flag -month: parse error
NAME:
appuio-odoo-adapter invoice - Create Odoo invoices from APPUiO Cloud
USAGE:
appuio-odoo-adapter invoice [command options] [arguments...]
OPTIONS:
--odoo-url value Odoo ERP URL in the form of https://username:password@host[:port]/db-name (default: <required>) [$OA_ODOO_URL]
--db-url value PostgreSQL URL in the form of postgres://username:password@host[:port]/db-name (default: <required>) [$OA_DB_URL]
--year value Year to generate the report for. (default: 0) [$OA_YEAR]
--month value Month to generate the report for. (default: 0) [$OA_MONTH]
--invoice-defaults-path value Path to a file with invoice defaults. [$OA_INVOICE_DEFAULTS_PATH]
--item-description-templates-path PRODUCT_SOURCE.gotmpl Path to a directory with templates. The Files must be named PRODUCT_SOURCE.gotmpl. (default: "description_templates/") [$OA_ITEM_DESCRIPTION_TEMPLATES_PATH]
--help, -h show help (default: false)
invalid value "08" for flag -month: parse error

Expected Behavior

The argument '08' gets parsed to integer 8, '09' gets parsed to integer 9.

Steps To Reproduce

Run appuio-odoo-adapter with argument "--month 08" or "--month 09".

Versions

v1.4.0

[ccremer]

I've opened a feature request at the CLI library: https://github.com/urfave/cli/issues/1462 As a quick fix, I think it should be safe enough to configure the Month flag as a StringFlag and do the integer parsing with base 10 in the action function, e.g. before using the month here: https://github.com/vshn/appuio-odoo-adapter/blob/8ffa1826fc39fdc2d52ec524fc025f4e52afcaa7/invoice_command.go#L95

[davidgubler]

Workaround implemented in https://github.com/appuio/component-appuio-cloud-reporting/pull/34

[glrf]

First of all thanks for catching and fixing this.

We discussed this in Tarazed. While we agree that this currently isn't intuitive, it's not exactly a bug.

In our opinion, the correct approach is to fix the component and to not unnecessarily prefix the month with a 0 (which you already did, thanks for fixing this).

So we don't think there is much reason to invest more time to make the adapter parse 0 prefixed integers in base 10.

[davidgubler]

it's not exactly a bug.

It is very much a bug, because prefixing single-digit months with '0' is common practice. Also, the fact that this problem is so subtle is another reason why it absolutely should be fixed, because somebody else might easily fall into this trap again if any part of the surrounding infrastructure needs to be touched in the future.

However there's not much point in investing much effort here, since @ccremer is currently working on a partial fix in the cli parser package which should make this configurable. Once that fix is available the fix in this project here should be trivial. So I would keep this open but not do anything for now.

[ccremer]

It is very much a bug, because prefixing single-digit months with '0' is common practice partial fix in the cli parser package which should make this configurable.

I agree this is a bug, but it doesn't have to be the cli parser's fault either. One might consider providing the month and year as separate arguments as "wrong use of API" (API being CLI here) since the CLI lib provides a dedicated Timestamp flag with a configurable date layout that accepts prefixing months with '0' (e.g. "yyyy-MM") exactly for this purpose. So it's definitely worth considering at some point to combine the month and year args into a timestamp flag and the bug at hand here is to have 2 separate arguments.

Anyway let's stop the blame game here, it doesn't help anyone now as we have a workaround.