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.

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: Default maximize_build_space to true #54

Merged
merged 1 commit into from
Jun 30, 2024

Conversation

gmpinder
Copy link
Member

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.

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.
@gmpinder gmpinder added the type: fix Iterations on existing features or infrastructure. label Jun 28, 2024
@gmpinder gmpinder self-assigned this Jun 28, 2024
@fiftydinar
Copy link
Collaborator

fiftydinar commented Jun 29, 2024

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
Copy link

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
Copy link
Member

xynydev commented Jun 29, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

xynydev commented Jun 30, 2024

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.

@fiftydinar fiftydinar merged commit bf3f2a9 into main Jun 30, 2024
@fiftydinar fiftydinar deleted the default-max-build-space-true branch June 30, 2024 10:05
@xynydev
Copy link
Member

xynydev commented Jun 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: fix Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants