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

Drop Puppet 5, add Puppet 7 and allow puppetlabs-concat 7.x #96

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

markruys
Copy link
Contributor

@markruys markruys commented Aug 5, 2022

Pull Request (PR) description

  • Support puppetlabs/concat 7.* and hence drop support for Puppet 5
  • Add support for Puppet 7
  • Clean up resources when ensure is set to absent (only partially implemented)

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

A few minor things to double check.

manifests/client.pp Outdated Show resolved Hide resolved
manifests/client.pp Outdated Show resolved Hide resolved
manifests/server.pp Outdated Show resolved Hide resolved
$server_ensure = 'absent'
} else {
$server_ensure = 'present'
}
Copy link
Member

Choose a reason for hiding this comment

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

In order to access $amanda::client::ensure and $amanda::server::ensure we must include amanda::client and include amanda::server first.

Copy link
Contributor Author

@markruys markruys Aug 18, 2022

Choose a reason for hiding this comment

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

Well, I'm not sure about this. I've tested (with Puppet 7), and the additional includes doesn't seem to be needed. Also, adding these includes changes the Amanda module behavior as the defaults are set to 'present'.

Copy link
Member

Choose a reason for hiding this comment

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

Well my point is that if the class amanda::server is not included, $amanda::server::ensure will be undef while the default value is 'present' when amanda::server is included… that's weird and seems fragile:

class { 'amanda::client':
}
# amanda::client include amanda
# `-> amanda include amanda::virtual
#     `-> amanda::virtual sees $amanda::server::ensure == undef
#                          and $amanda::client::ensure == 'present'
class { 'amanda::server':
}
class { 'amanda::server':
}
# amanda::server include amanda
# `-> amanda include amanda::virtual
#     `-> amanda::virtual sees $amanda::server::ensure == 'present'
#                          and $amanda::client::ensure == undef
class { 'amanda::client':
}
class { 'amanda':
}
# amanda include amanda::virtual
# `-> amanda::virtual sees $amanda::server::ensure == undef
#                      and $amanda::client::ensure == undef
class { 'amanda::client':
}
class { 'amanda::server':
}

I have no experience with amanda, but this does not look right 😨

metadata.json Outdated Show resolved Hide resolved
@smortex smortex changed the title Support concat 7.* and Puppet 7 Drop Puppet 5, add Puppet 7 and allow puppetlabs-concat 7.x Aug 18, 2022
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.

2 participants