Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from
+10 −10
Closed

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ab6f9b
fix: actually use CONFIG_TOML_FILE param if provided
pastequo Jul 30, 2024
2adf957
Try to make this make more sense
ralphbean Jul 30, 2024
536965b
Use empty config.toml file if one not found
ralphbean Jul 31, 2024
cecd998
Make this log line more correct
ralphbean Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions task/build-vm-image/0.1/build-vm-image.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,19 +155,19 @@ spec:
ssh -v $SSH_ARGS "$SSH_HOST" mkdir -p "$BUILD_DIR/workspaces" "$BUILD_DIR/scripts" "$BUILD_DIR/tmp" "$BUILD_DIR/tekton-results" "$BUILD_DIR/entitlement"

if [ ! -n "${CONFIG_TOML_FILE}" ]; then
echo "No CONFIG_TOML_FILE specified"
echo "No CONFIG_TOML_FILE specified. Assuming config.toml"
export CONFIG_TOML_FILE=config.toml
if [ -f /var/workdir/source/config.toml ]; then
echo "Using the config.toml file found in the repository root!"
echo " Remove the config.toml file or set params.CONFIG_TOML_FILE to another file to prevent using config.toml."
else
echo "No config.toml file found. Using an empty configuration."
touch /var/workdir/source/$CONFIG_TOML_FILE
fi
fi
echo "Using the following config.toml file $CONFIG_TOML_FILE:"
cat /var/workdir/source/$CONFIG_TOML_FILE

if [ -f "/var/workdir/source/${CONFIG_TOML_FILE}" ]; then
echo "Using the ${CONFIG_TOML_FILE} file found in the repository."
else
echo "No ${CONFIG_TOML_FILE} file found. Using an empty configuration."
touch "/var/workdir/source/${CONFIG_TOML_FILE}"
fi
Copy link
Contributor

@chmeliik chmeliik Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@chmeliik chmeliik Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


echo "Using the following ${CONFIG_TOML_FILE} file:"
cat /var/workdir/source/$CONFIG_TOML_FILE

rsync -ra "/var/workdir/source/$CONFIG_TOML_FILE" "$SSH_HOST:$BUILD_DIR/config.toml"
rsync -ra "$HOME/.docker/" "$SSH_HOST:$BUILD_DIR/.docker/"
Expand Down
Loading