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

feat(#5864): Added configuration for sizeLimit on empty-dirs in mount trait #5865

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hernanDatgDev
Copy link
Contributor

The default SizeLimit on mount trait empty-dir volumes is now configurable with the following syntax:
trait.mount. empty-dirs=myVolumeName:/container/path:sizeLimit

Where sizeLimit is a normal resource quantity e.g. 1Gi, 500Mi, etc.

I considered something similar to components where we add configurations after a "?" however since the empty-dir volume source only takes 2 configurations:
SizeLimit
Medium

I figured it would be easier to just continue the ":" separation syntax. Please let me know your thoughts on this approach

Release Note

NONE

@hernanDatgDev hernanDatgDev marked this pull request as draft September 25, 2024 19:43
@hernanDatgDev
Copy link
Contributor Author

I need to update the documentation for the syntax on the trait still unless you have any concerns with the approach and I should change it. @squakez

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I think the parsing is getting too complicated and out of the original scope. Probably it makes sense that we have a separate section in the mount trait that takes care to parse and to add volumes coming from the empty-dir parameter independently by the rest. In that way we can treat them appropriately adding any possible future syntax change.

Beside that, it would be good to set a default value on resource when the user is not providing it (either 1GB or anything lower).

@hernanDatgDev hernanDatgDev marked this pull request as ready for review October 1, 2024 19:07
@hernanDatgDev
Copy link
Contributor Author

Made the changes so that logic is contained mostly within the mount trait. Parsing is kept to a minimum and I'd just need to update the syntax documentation if the logic looks good @squakez

@hernanDatgDev hernanDatgDev marked this pull request as draft October 1, 2024 19:08
Copy link
Contributor

github-actions bot commented Oct 3, 2024

✔️ Unit test coverage report - coverage increased from 45% to 45.1% (+0.1%)

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

LGTM. Please add the doc changes and have a look at the lint failure.

@hernanDatgDev
Copy link
Contributor Author

@squakez I've run make generate to handle the documentation update but there are a lot of additional changes that result from this change, specifically to the CRD definitions. They updates do include my updated documentation though.

Any thoughts? I can always revert this commit.

@hernanDatgDev hernanDatgDev marked this pull request as ready for review October 8, 2024 19:43
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I see you're using an old controller-gen version. You need to upgrade to a more recent one (likely 0.15.0) and redo the generation.

@squakez
Copy link
Contributor

squakez commented Oct 15, 2024

Hi @hernanDatgDev how this one is going? I am planning to release 2.5.0 for next week, so, we should have this ready by then to include in the release.

@squakez squakez mentioned this pull request Oct 15, 2024
@squakez squakez linked an issue Oct 20, 2024 that may be closed by this pull request
@hernanDatgDev
Copy link
Contributor Author

@squakez I'm sorry about the delay. I will get this updated and ready as soon as I can.

@hernanDatgDev hernanDatgDev force-pushed the feature/5864/configureable-sizeLimit-on-empty-dirs branch from 8e9b1e9 to 13883c9 Compare October 24, 2024 00:41
@squakez
Copy link
Contributor

squakez commented Oct 24, 2024

Let's wait for the checks to run. For now, don't worry about the autogenerated documentation, we have a nightly task taking care to update. However, for the future, we need to take care of synchronizing the tooling automatically, so, I'm opening a separate issue to take care of that.

@squakez
Copy link
Contributor

squakez commented Oct 24, 2024

pkg/trait/mount.go:284:65 godot Comment should end in a period

Please, fix and make lint locally to run the validation and see no more errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give configurable SizeLimit to emptyDir volumes in mount trait
3 participants