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

Add support for archive install on EL #190

Merged
merged 2 commits into from Sep 26, 2022

Conversation

m0dular
Copy link
Contributor

@m0dular m0dular commented Aug 17, 2022

This PR allows for installing from an archive source on EL. It also exposes the manage_user parameter, which was previously only defined in params.pp.

spec/setup_acceptance_node.pp Outdated Show resolved Hide resolved
@@ -121,6 +133,41 @@
}
Yumrepo['influxdata'] -> Package[$telegraf::package_name]
}
elsif $telegraf::manage_archive {
Copy link
Member

Choose a reason for hiding this comment

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

This makes me think manage_repo and manage_archive are mutually exclusive, it could be cool to detect this and raise an error rather than sometimes installing from a repo and sometimes from an archive… Not a blocker for this PR anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did think about that when adding this, but didn't really want to rewrite a bunch of the logic. I added the warnings for unsupported install methods, because I tried to install from archive on CentOS and it just fell through the if statement without installing any packages.

@smortex smortex changed the title El archive install Add support for archive install on EL Aug 20, 2022
@smortex smortex added the enhancement New feature or request label Aug 20, 2022
@@ -83,6 +83,9 @@
# [*manage_archive*]
# Boolean. Whether or not to manage InfluxData's tar archive.
#
# [*manage_user*]
# Boolean. Whether or not to manage the 'telegraf' user when installing from archive.
Copy link
Member

Choose a reason for hiding this comment

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

for new parameters we would like to have the puppet-strings style (that allows us to autogenerate documentation):

Suggested change
# Boolean. Whether or not to manage the 'telegraf' user when installing from archive.
# @param manage_user Whether or not to manage the 'telegraf' user when installing from archive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we keep the existing param in brackets above this line, [*manage_user*]? In newer modules it looks like we do this:

# @param param_name
#   Description of param

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, with or without the new line, using @param is better 👍. The top of the comment also need to be adjusted to remove the markdown syntax (is it markdown? Y mean the = strings are no something puppet-string likes).

Puppet has some guidance for documenting classes here:
https://puppet.com/docs/puppet/7/puppet_strings_style.html#puppet_strings_style-strings-style-classes-types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've become reliant on pdk validate for this, I guess.

@m0dular m0dular force-pushed the EL-archive-install branch 2 times, most recently from 456d51b to 4282300 Compare August 23, 2022 15:46
@bastelfreak
Copy link
Member

please rebase after #191 is merged. that should fix the failing ubuntu jobs

@m0dular
Copy link
Contributor Author

m0dular commented Sep 20, 2022

Done 👍

@bastelfreak bastelfreak merged commit 9d3ae15 into voxpupuli:master Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants