-
Notifications
You must be signed in to change notification settings - Fork 236
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
Attempt auto-cleanup of otp_builds
and otp_installations
, at every run
#490
Attempt auto-cleanup of otp_builds
and otp_installations
, at every run
#490
Conversation
kerl
Outdated
@@ -2099,6 +2099,39 @@ get_active_path() { | |||
fi | |||
} | |||
|
|||
fix_otp_installations() { | |||
if [ -f "$KERL_BASE_DIR"/otp_installations ]; then | |||
inexisting_names="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna point out that "inexisting" isn't a word (well, actually it is, but it's obsolete and doesn't mean what you think it means). You want "nonexistant" or possibly just "missing".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Naming, right? (I guess I "translated" directly from portuguese "inexistente") Thanks. I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kerl
Outdated
@@ -2099,6 +2099,38 @@ get_active_path() { | |||
fi | |||
} | |||
|
|||
fix_otp_installations() { | |||
if [ -f "$KERL_BASE_DIR"/otp_installations ]; then | |||
nonexisting_names="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would use "missing" - that's the more natural word for this situation 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Naming, right? I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebasing current changes onto the main branch as there's recently been quite a few changes (i.e. I wanna check CI is still Ok atm). |
4ed5f5a
to
bcdc710
Compare
I made a few changes and made a PR to your fork. Let me know what you think. 😄 |
I'll make some comments there. Edit: at the same time, I'm waiting on @jadeallenx's re-review. Edit: I'll import some of your PR's variable name changes; it introduces logging (not sure it's wanted); and it moves code around (for which I'm still lacking a reply, but didn't do because of that). |
Sorry it took such a long time to get back to this - let's merge it if it's ready |
Thanks, @jadeallenx. I'll rebase on the main branch and force-push. If it passes CI I'm Ok with the result and I'll merge. |
60d11e5
to
fea2792
Compare
Description
We attempt (to start - open to discussion) to clean
otp_builds
andotp_installations
every timekerl
is run.Closes #216.
Closes #236.