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 debian repo support #37

Merged

Conversation

SimonHoenscheid
Copy link
Contributor

@SimonHoenscheid SimonHoenscheid commented Aug 22, 2022

@SimonHoenscheid SimonHoenscheid requested a review from a team as a code owner August 22, 2022 13:54
@puppet-community-rangefinder
Copy link

influxdb is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

This module is declared in 0 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@SimonHoenscheid SimonHoenscheid mentioned this pull request Aug 22, 2022
@m0dular
Copy link
Contributor

m0dular commented Aug 22, 2022

Thank you for opening these PRs! I'll start taking a look at them today.

Copy link
Contributor

@m0dular m0dular left a comment

Choose a reason for hiding this comment

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

Thanks for this one, I've been meaning to get to this issue. The code looks good, but like #36 could we only have the commits related to the feature in the PR, d362b1f in this case?

@MartyEwings MartyEwings added the enhancement New feature or request label Aug 23, 2022
@SimonHoenscheid SimonHoenscheid force-pushed the shoenscheid_debian_repo_support branch from d362b1f to 6010db7 Compare August 23, 2022 13:32
@SimonHoenscheid
Copy link
Contributor Author

@m0dular I included the debian repo support feature here, because it contains variables used by this feature.

@m0dular
Copy link
Contributor

m0dular commented Aug 23, 2022

Ah that's right, they both use the repository related variables. I'll kick off the acceptance tests, and I'll find some time to add a couple unit tests to cover it. Unless you feel like doing those yourself :P

@m0dular
Copy link
Contributor

m0dular commented Aug 23, 2022

I always forget this, but for spec tests we need module deps in .fixtures.yml. Could you add the apt module to the forge_modules key here:

fixtures:
forge_modules:
archive: "puppet/archive"
yumrepo: "puppetlabs/yumrepo_core"

One of the spec tests is failing because it expects the gpg url to be

https://repos.influxdata.com/influxdb2.key https://repos.influxdata.com/influxdb.key

But it only contains the second entry. I don't remember why I set it up that way, but I think we need the influxdb2.key? I'm not 100% sure, but would it make sense to update the module data to have both urls?

manifests/init.pp Show resolved Hide resolved
@SimonHoenscheid
Copy link
Contributor Author

@m0dular The repo descriptions just references one key:

https://portal.influxdata.com/downloads/

hiera.yaml Outdated Show resolved Hide resolved
data/os/CentOS.yaml Outdated Show resolved Hide resolved
@SimonHoenscheid SimonHoenscheid force-pushed the shoenscheid_debian_repo_support branch 2 times, most recently from 3935dbe to 7f03904 Compare August 24, 2022 07:22
@bastelfreak
Copy link
Collaborator

I took a look at the gpg keys:

bastelfreak@bastelfreak-nb /tmp $ md5sum influxdb*
e7b6670153b90766ae2f5dc7bdc61c6a  influxdb2.key
e38c47ef9138d841ee94f471f0ab01fb  influxdb.key
e38c47ef9138d841ee94f471f0ab01fb  influxdb.key.old
bastelfreak@bastelfreak-nb /tmp $

I didn't find an official statement from influxdb which key has which purpose, but:

@m0dular
Copy link
Contributor

m0dular commented Aug 24, 2022

The acceptance tests would show us whether removing the gpg key would be an issue, but I also think we should keep the GPG url the same for now. That can be revisited later if we need to.

@SimonHoenscheid
Copy link
Contributor Author

@m0dular Only thing to think about: Yumrepo doesn't seem to have a Problem with two keys AFAIK I dont know if Apt has. so my suggestion would be to go with the old key for Ubuntu and Debian

@m0dular
Copy link
Contributor

m0dular commented Aug 25, 2022

Sounds good. If you can push a commit that fixes the $repo_url error and adds the key back I'll kick off the acceptance tests. Should we have both keys in common.yaml and add the old key to Debian.yaml and Ubuntu.yaml?

@SimonHoenscheid
Copy link
Contributor Author

Sounds good. will do.

@SimonHoenscheid SimonHoenscheid force-pushed the shoenscheid_debian_repo_support branch 2 times, most recently from 50c1d02 to 0c394ee Compare August 26, 2022 07:04
@SimonHoenscheid
Copy link
Contributor Author

@m0dular PR updated :-)

@MartyEwings
Copy link
Contributor

MartyEwings commented Aug 26, 2022

I think the failures a related to the fact that the Debian an ubuntu repos for influx db only keep the latest version in them, we may need to specify latest or something for these OS's

https://repos.influxdata.com/ubuntu/dists/focal/stable/binary-arm64/Packages

I came across this before its really curious why they do this

@SimonHoenscheid
Copy link
Contributor Author

@MartyEwings I will add that to the data

@SimonHoenscheid SimonHoenscheid force-pushed the shoenscheid_debian_repo_support branch from 0c394ee to 854eeab Compare August 26, 2022 11:36
@SimonHoenscheid
Copy link
Contributor Author

@MartyEwings @m0dular done. Do you want me to lint the code or should I rebase after #31 merged? I am also fine with closing #31 and linting #36 and #37

@@ -105,7 +125,7 @@

package {'influxdb2':
Copy link
Contributor

Choose a reason for hiding this comment

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

There were some errors in the acceptance tests related to the ssl files being applied before the /etc/influxdb directory exists, and influxdb.service.d being applied before the influxdb user exists. I don't know why this happens on Ubuntu and not elsewhere, but I tested this change and it worked.

    $package_before = if $use_ssl and $manage_ssl {
      [
        File['/etc/influxdb/cert.pem', '/etc/influxdb/key.pem', '/etc/influxdb/ca.pem', '/etc/systemd/system/influxdb.service.d'],
        Service['influxdb']
      ]
    }
    else {
      Service['influxdb']
    }

    package {'influxdb2':
      ensure  => $version,
      require => $package_require,
      before  => $package_before,
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @SimonHoenscheid, we're still seeing these failures in acceptance tests.

Error: Could not set 'file' on ensure: No such file or directory - A directory component in /etc/influxdb/cert.pem20220913-19386-1aod2be.lock does not exist or is a dangling symbolic link (file: /etc/puppetlabs/code/environments/production/modules/influxdb/manifests/init.pp, line: 200)

Can you try adding this code so the package is installed before we try to set up the certs?

@m0dular
Copy link
Contributor

m0dular commented Aug 26, 2022

I went ahead and merged #31 because it's only linting. That created a merged conflict here on the closing ) of the parameters list for... some reason.

For #36 we could either apply the same $repo_url and influxdb::repo_gpg_key_url changes there and merge it, or just have those changes in this PR. Whichever way is easier works for me.

@SimonHoenscheid
Copy link
Contributor Author

This PR is waiting for #36 to be merged. Afterwards this PR can be rebased.

@SimonHoenscheid SimonHoenscheid force-pushed the shoenscheid_debian_repo_support branch from 854eeab to 08cf994 Compare September 2, 2022 12:15
@SimonHoenscheid
Copy link
Contributor Author

I already rebased this PR. It can be merged after #36

@SimonHoenscheid SimonHoenscheid force-pushed the shoenscheid_debian_repo_support branch from 08cf994 to 7e4f062 Compare September 16, 2022 06:55
@m0dular m0dular merged commit 7e4f062 into puppetlabs:main Sep 26, 2022
@m0dular
Copy link
Contributor

m0dular commented Sep 26, 2022

I added a commit that includes the $package_require code and an update to the spec tests, merged in #40. Thanks for the contribution!

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.

Add repository for Debian/Ubuntu
4 participants