adm: Fix panic on negative value #1127

Anoke · 2024-05-08T16:50:41Z

Anoke commented

2024-05-08 16:50:41 +00:00

If there was an error (it tried parse negative value in ParseUint), the If condition executes and by trying send an error it tries to get value args[1] which is not exists. I changed it to args[0] and it works. (args = [ExecFeeFactor=-1])

achuprov approved these changes 2024-05-13 07:01:36 +00:00

cmd/frostfs-adm/internal/modules/morph/policy/policy.go Outdated

					
				@ -49,3 +49,3 @@

						value, err := strconv.ParseUint(v, 10, 32)

						if err != nil {

							return fmt.Errorf("can't parse parameter value '%s': %w", args[1], err)

							return fmt.Errorf("can't parse parameter value '%s': %w", args[0], err)

achuprov commented

2024-05-13 07:00:34 +00:00

Can you change the commit message format to: [#1067] frostfs-adm: Fix panic on negative value.
Can you include all changes in one commit?

Can you change the commit message format to: `[#1067] frostfs-adm: Fix panic on negative value`. Can you include all changes in one commit?

👍 2

fyrchik commented

2024-05-13 07:52:57 +00:00

Most likely, it should be args[i], we do this inside the loop.

Most likely, it should be `args[i]`, we do this inside the loop.

👍 1

achuprov commented

2024-05-13 11:08:50 +00:00

I meant the commit message, not the title of the Pull Request. Please use the command:
git commit -s -m "[#1067] frostfs-adm: Fix panic on negative value"
If you're using an IDE to make commits, ensure that commit signing is enabled in the settings.

I meant the commit message, not the title of the Pull Request. Please use the command: ```git commit -s -m "[#1067] frostfs-adm: Fix panic on negative value"``` If you're using an IDE to make commits, ensure that `commit signing` is enabled in the settings.

👍 1

fyrchik marked this conversation as resolved

Anoke changed title from ~~frostfs-adm panics on negative values #1067~~ to [#1067] frostfs-adm: Fix panic on negative value

2024-05-13 09:35:25 +00:00

Anoke force-pushed negative-value-issue from e18a38c85e to 0b7d706450

2024-05-13 09:48:33 +00:00

Compare

fyrchik commented

2024-05-13 11:00:33 +00:00

Commit message still needs care.

fyrchik requested review from storage-core-committers 2024-05-13 11:06:40 +00:00

fyrchik requested review from storage-core-developers 2024-05-13 11:06:40 +00:00

Anoke force-pushed negative-value-issue from 0b7d706450 to c62178004a

2024-05-13 11:29:42 +00:00

Compare

Anoke force-pushed negative-value-issue from c62178004a to d7feb1019e

2024-05-13 11:57:12 +00:00

Compare

fyrchik commented

2024-05-13 12:11:17 +00:00

@achuprov made a small mistake, by convention we use adm:, not frostfs-adm: for prefix.

Also, signoff is mandatory, git commit has --signoff option which adds a special mark to commit, see the DCO action which fails.

@achuprov made a small mistake, by convention we use `adm:`, not `frostfs-adm:` for prefix. Also, signoff is mandatory, `git commit` has `--signoff` option which adds a special mark to commit, see the `DCO` action which fails.

👍 1

fyrchik changed title from ~~[#1067] frostfs-adm: Fix panic on negative value~~ to adm: Fix panic on negative value

2024-05-13 12:11:42 +00:00

fyrchik commented

2024-05-13 12:11:59 +00:00

Other than that, LGTM

👍 1

Anoke force-pushed negative-value-issue from 8b791b3675 to c4a6e3ec8d

2024-05-13 12:59:08 +00:00

Compare

Anoke force-pushed negative-value-issue from c4a6e3ec8d to 0932c9f7bf

2024-05-13 13:11:52 +00:00