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

Lazily load tftp directories #674

Merged
merged 2 commits into from
Jun 1, 2021
Merged

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented May 4, 2021

This change makes tftp directories lazy. It does that by making the parameters undef by default and determine them if tftp is actually included.

The main motivation is that f849d12 changed the tftp module. Prior to it, the logic was duplicated. With that change, it became dynamic which also made it a hard dependency. Technically it already was since it's always been listed in metadata.json. However, it functioned without it if tftp was false. With this change it really becomes optional.

Alternative to #673.

@ekohl
Copy link
Member Author

ekohl commented May 4, 2021

It now fails since it's used in templates:

:root_dir: <%= scope.lookupvar('foreman_proxy::tftp_root') %>

:tftproot: <%= scope.lookupvar("foreman_proxy::tftp_root") %>

Before I start diving into this, I'd like to know if people agree this is a good path forward.

@ehelms
Copy link
Member

ehelms commented May 5, 2021

This makes sense to me to only perform necessary actions, set necessary variables if a feature is enabled. I would proceed ahead with diving in.

This change makes tftp directories lazy. It does that by making the
parameters undef by default and determine them if tftp is actually
included.

The main motivation is that f849d12
changed the tftp module. Prior to it, the logic was duplicated. With
that change, it became dynamic which also made it a hard dependency.
Technically it already was since it's always been listed in
metadata.json. However, it functioned without it if tftp was false. With
this change it really becomes optional.
This command requires the directory to exist. This has always been
needed, but previously it mostly happened to install in the right order
so it was missed.
@ekohl
Copy link
Member Author

ekohl commented May 27, 2021

This is now ready for review. The one test that's failing is expected since there's no Puppet 5 AIO package on Ubuntu 20.04.

@ehelms ehelms merged commit 62a96f7 into theforeman:master Jun 1, 2021
@ekohl ekohl deleted the optional-tftp branch August 4, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants