blue-build / github-action

Reusable GitHub Action to build custom images
Apache License 2.0
14 stars 4 forks source link

fix: Default maximize_build_space to true #54

Closed gmpinder closed 5 months ago

gmpinder commented 5 months ago

This comes as a realization that we've had many several users run into this issue only to be told they need to change an input for the action. I feel that defaulting this to true is a saner default if only to allow users to build without eventually running into this problem as they increase their image size. It seems to confuse more than help with default false.

fiftydinar commented 5 months ago

We also thought on this same topic before & decided to -1 on this, since it increases the time of building the image & at the time, people rarely encountered this issue.

But now, I think it's better to enable this by default, since there can be some non-cryptic errors, where users didn't know how to proceed & had to ask for the help in BlueBuild Discord server. Increased build times are a worth sacrifice to solve this issue.

Since this step can't be done dynamically & reliably, to detect if this step is truly required or not per image-size, I'm in.

Let's also wait for other peoples response.

+1

Janybanny commented 5 months ago

This comes from someone, who doesn't know a lot about GitHub actions and how everything works, but can you detect the not enough space errors consistently and then trigger something because of it?

I feel like the best of both worlds would be to disable it by default, but enable it automatically when a user encounters such an error.

This would also need a third state of "force-disabled" or something similar, if a user doesn't want it to get automatically enabled.

xynydev commented 5 months ago

I agree with @Janybanny here. It's not possible to auto-enable it, though. I would still prefer a way to recognize the issue and when encountered print an error message with instructions on how to solve it.

Running with this option by default would slow down everyone's builds by a few minutes, slowing down the dev feedback loop even more, and hurting UX. It's also good to remember that the people who come ask about this error on discord are a marginal slice of the userbase, and we don't know how much of actual users need this turned on. Most are likely not adding that much, though.

That is why I tend to view this as a documentation problem and not a defaults problem. Regardless of defaults, the error should be documented better on our website and the option added to the template build.yml with an explanation of when to change it.

That should leave it down to whether we can detect the error and print a descriptive error message, if not can we trust that people will find the necessary information in the docs, and if not either of those, enabling it by default would be best.

gmpinder commented 5 months ago

Running with this option by default would slow down everyone's builds by a few minutes, slowing down the dev feedback loop even more, and hurting UX. It's also good to remember that the people who come ask about this error on discord are a marginal slice of the userbase, and we don't know how much of actual users need this turned on. Most are likely not adding that much, though.

I'd argue that the better UX would be for it to not fail in the first place. Creating speed bumps where it's a hard stop vs a longer build time is worth it to let the user focus on their recipe.

That should leave it down to whether we can detect the error and print a descriptive error message

Unfortunately won't work. Docker doesn't say it it fails because of disk space. It's up to the process in the build to notice and report it, and rpm-ostree just says error: Error -1 running transaction. Not helpful.

If our focus for BlueBuild is to make creating your own Linux images as simple as possible for less technical users, defaults that allow smoother operation is more preferable to optimizing build times. Users just want it to work.

gmpinder commented 5 months ago

That's not to say we shouldn't also update our documentation while we're at it. This isn't an either or situation.

xynydev commented 5 months ago

Ok, if detecting the error isn't reliably possible, and the error messages do not make the issue any more clearer or searchable, I probably agree then.

As I said previously:

That should leave it down to whether we can detect the error and print a descriptive error message, if not can we trust that people will find the necessary information in the docs, and if not either of those, enabling it by default would be best.

With the error message in the example, I think the answer to can we trust that people will find the necessary information in the docs is also no. I think I've seen different error messages too with the same error, but I guess it depends on what module causes the failure, which would make the situation a whole lot less clear too.

xynydev commented 5 months ago

Made the docs changes, we may consider this matter settled.