fix: clean up logic around config.toml handling #1222

ralphbean · 2024-07-30T18:58:07Z

No description provided.

ralphbean · 2024-07-30T19:16:15Z

See also https://gitlab.com/redhat/rhel-ai/disk-images/nvidia-bootc/-/merge_requests/56

arewm · 2024-07-30T19:43:00Z

task/build-vm-image/0.1/build-vm-image.yaml

+          echo "Using the ${CONFIG_TOML_FILE} file found in the repository root!"
+        else
+          echo "No ${CONFIG_TOML_FILE} file found in the repository. Set params.${CONFIG_TOML_FILE}_FILE to a valid config.toml file."
+          exit 1


This will make it impossible for the task to run if there is no file regardless as to whether the parameter was specified or not. Is that desired?

I think so. Right? Like, what's the behavior of bib if there is literally no config.toml at all?

This is effectively a breaking change. If we want to make it explicit like this then will we have to bump the version?

I don't know if the file is missing, but bib is fine if the file is empty

I'm not interested in introducing a breaking change. I'm just trying to clean it up.

If bib is happy if the file is empty, I'll drop this part.

OK - I revised this to drop the exit 1 and restore the touch line. (Now I understand what that line was for. I assumed bib would balk at an empty config.toml file.)

task/build-vm-image/0.1/build-vm-image.yaml

arewm · 2024-07-30T19:45:35Z

task/build-vm-image/0.1/build-vm-image.yaml

+        if [ -f "/var/workdir/source/${CONFIG_TOML_FILE}" ]; then
+          echo "Using the ${CONFIG_TOML_FILE} file found in the repository root!"
+        else
+          echo "No ${CONFIG_TOML_FILE} file found in the repository. Set params.${CONFIG_TOML_FILE}_FILE to a valid config.toml file."


Suggested change

echo "No ${CONFIG_TOML_FILE} file found in the repository. Set params.${CONFIG_TOML_FILE}_FILE to a valid config.toml file."

echo "No ${CONFIG_TOML_FILE} file found in the repository. Set params.CONFIG_TOML_FILE to a valid config.toml file."

?

pastequo · 2024-07-31T07:42:19Z

Maybe the PR title doesn't reflect what you want to achieve, but the CONFIG_TOML_FILE is taken into account (at least in our pipeline it seems ok)

chmeliik · 2024-07-31T08:09:09Z

Maybe the PR title doesn't reflect what you want to achieve, but the CONFIG_TOML_FILE is taken into account (at least in our pipeline it seems ok)

Looking at https://gitlab.com/redhat/rhel-ai/disk-images/nvidia-bootc/-/merge_requests/56, it's not up to date with the current build-definitions main. Maybe the behavior is already fixed?

A very nasty bug in the gitlab version is

if [! -n "${CONFIG_TOML_FILE}" ]; then

This is just a much harder to spot if nonexistent-command (if you're not running ShellCheck, which spots it for you)

#!/bin/bash
set -e

if [! -n "" ]; then
    echo 'true'
else
    echo 'false'
fi

$ bash /tmp/x.bash
/tmp/x.bash: line 4: [!: command not found
false

Signed-off-by: Ralph Bean <[email protected]>

The logic here was pretty confusing. Trying to straighten it out.

Andrew pointed out in code review that it's too specific to claim that it is found in the repository root. It could be in a subdir.

chmeliik · 2024-07-31T13:51:33Z

task/build-vm-image/0.1/build-vm-image.yaml

+        else
+          echo "No ${CONFIG_TOML_FILE} file found. Using an empty configuration."
+          touch "/var/workdir/source/${CONFIG_TOML_FILE}"
+        fi


With the old version of the task, explicitly passing a non-existent CONFIG_TOML_FILE would result in a failure (intentionally) rather than ignoring the problem and creating an empty config.

IMO that's the saner behavior that one would expect from most tools, but there seems to be a natural pull towards creating the empty config instead (both in your and in Andrew's PRs 😄 )

I'm fine with it either way, the log message should make it clear why e.g. a mis-spelled config file path is being ignored

Aha. Now that you say that, I do remember those iterations two weeks back. I think that the reason that both @ralphbean and myself did that is because it enables cleaner/clearer logic.

Once I tried to allow failures when a nonexistent file is specified, the logic became more confusing.

See the table for intended behavior from the prior PR: #1171 (comment)

I wouldn't say it enables cleaner logic, it's just a touch easier to make it clean. Can still be clean and behave nicely though

if [ -z "$CONFIG_TOML_FILE" ]; then CONFIG_TOML_FILE=config.toml touch "/var/workdir/source/$CONFIG_TOML_FILE" elif ! [ -f "/var/workdir/source/$CONFIG_TOML_FILE" ]; then echo "Config file doesn't exist: $CONFIG_TOML_FILE" exit 1 fi

I fear I've made a mess. I wasn't aware of the conversations you guys had the other week about this and I assumed this section of code was just carelessly written in the first place during fast-hacking to make vm building work in early June, when in reality you guys had really thought it through. The code was written more recently than I knew!

My real goal here was to get https://gitlab.com/redhat/rhel-ai/disk-images/nvidia-bootc/-/commit/3ab9e272069e9469abc2ace36bf11b63a438f456 to happen - and it did.

I'm inclined to drop this PR without merging it, just to let it exist as you guys defined it in #1171 (comment). WDYT?

@ralphbean , @chmeliik 's suggestion above is similar to what you proposed. It still defines the failure criteria and clearly describes why a failure happens instead of falling back to other processes to fail (i.e. rsync). I think this would still be an improvement.

Ack, if we adopt that - we'll need a version bump per this comment, true?

No, If the only failure is a user specifying a file path that does not exist then that is consistent. The breaking change previously is if no file was specified. We would not create an empty file for them.

ralphbean · 2024-09-14T13:25:42Z

Hey guys, I'm going to drop this. Focusing on other things.

ralphbean requested review from arewm and mmorhun July 30, 2024 19:16

ralphbean force-pushed the fix-config-toml branch from 1131462 to 875880a Compare July 30, 2024 19:29

arewm reviewed Jul 30, 2024

View reviewed changes

task/build-vm-image/0.1/build-vm-image.yaml Outdated Show resolved Hide resolved

arewm reviewed Jul 30, 2024

View reviewed changes

pastequo and others added 2 commits July 31, 2024 08:29

fix: actually use CONFIG_TOML_FILE param if provided

4ab6f9b

Signed-off-by: Ralph Bean <[email protected]>

Try to make this make more sense

2adf957

The logic here was pretty confusing. Trying to straighten it out.

ralphbean changed the title ~~fix: actually use CONFIG_TOML_FILE param if provided~~ fix: clean up logic around config.toml handling Jul 31, 2024

ralphbean added 2 commits July 31, 2024 08:32

Use empty config.toml file if one not found

536965b

Make this log line more correct

cecd998

Andrew pointed out in code review that it's too specific to claim that it is found in the repository root. It could be in a subdir.

ralphbean force-pushed the fix-config-toml branch from 875880a to cecd998 Compare July 31, 2024 12:33

arewm approved these changes Jul 31, 2024

View reviewed changes

chmeliik reviewed Jul 31, 2024

View reviewed changes

ralphbean closed this Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: clean up logic around config.toml handling #1222

fix: clean up logic around config.toml handling #1222

ralphbean commented Jul 30, 2024

ralphbean commented Jul 30, 2024

arewm Jul 30, 2024

ralphbean Jul 30, 2024

arewm Jul 30, 2024

pastequo Jul 31, 2024

ralphbean Jul 31, 2024

ralphbean Jul 31, 2024

arewm Jul 30, 2024

pastequo commented Jul 31, 2024

chmeliik commented Jul 31, 2024 •

edited

Loading

chmeliik Jul 31, 2024 •

edited

Loading

arewm Jul 31, 2024

arewm Jul 31, 2024

chmeliik Jul 31, 2024 •

edited

Loading

ralphbean Jul 31, 2024

chmeliik Aug 1, 2024

arewm Aug 1, 2024

ralphbean Aug 2, 2024

arewm Aug 5, 2024

ralphbean commented Sep 14, 2024

	echo "No ${CONFIG_TOML_FILE} file found in the repository. Set params.${CONFIG_TOML_FILE}_FILE to a valid config.toml file."
	echo "No ${CONFIG_TOML_FILE} file found in the repository. Set params.CONFIG_TOML_FILE to a valid config.toml file."

fix: clean up logic around config.toml handling #1222

fix: clean up logic around config.toml handling #1222

Conversation

ralphbean commented Jul 30, 2024

ralphbean commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pastequo commented Jul 31, 2024

chmeliik commented Jul 31, 2024 • edited Loading

chmeliik Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmeliik Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralphbean commented Sep 14, 2024

chmeliik commented Jul 31, 2024 •

edited

Loading

chmeliik Jul 31, 2024 •

edited

Loading

chmeliik Jul 31, 2024 •

edited

Loading