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

debian: service: fix ensure parameter usage #1095

Merged
merged 5 commits into from
Jan 27, 2023
Merged

Conversation

damonbreeden
Copy link
Contributor

ensure is defined but not used anywhere, no clear way to ensure the service is running (and makes it impossible to do so outside the module)

`ensure` is defined but not used anywhere, no clear way to ensure the service is running
(and makes it impossible to do so outside the module)
@damonbreeden damonbreeden requested a review from a team as a code owner December 2, 2022 16:17
@CLAassistant
Copy link

CLAassistant commented Dec 2, 2022

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

firewall::linux::debian is a class

that may have no external impact to Forge modules.

This module is declared in 106 of 580 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.

@jordanbreen28
Copy link
Contributor

Hi @damonbreeden
Thanks for your work on this one, however it looks as though your change has introduced some spec test failures.
Are you able to address these failures?
Thanks

@kjetilho
Copy link
Contributor

kjetilho commented Dec 6, 2022

A rebase on "main" will probably fix the checks (and trigger a new run of checks)

@damonbreeden
Copy link
Contributor Author

hi, your test failure expects the service to be unmanaged but IMO this is an oversight, since debian is the only distro that expects this (all others are set to respect the ensure param correctly) and since we define ensure => running in the class params but it's never used anywhere in the class

this makes it impossible to actually manage the running state of the service with puppet, since we define it here as undef so we can't define it anywhere else

@jordanbreen28
Copy link
Contributor

hi @damonbreeden, thanks for providing some more context surrounding the test failures.

Please feel free to update the debian specific spec tests so that the now pass with this change. (When you have the free time!)

I suspect the failing RHEL-9 issues are due to your branch being outdated, and a fix has since been merged into main for this.

Thanks for your work on this one.

@damonbreeden
Copy link
Contributor Author

hi @jordanbreen28 pls see latest commits, i think they address the spec tests correctly

@damonbreeden
Copy link
Contributor Author

damonbreeden commented Jan 6, 2023

sorry, spoke too quickly
seems like it's failing on an unknown variable now, but (should be) unrelated to this PR

       Evaluation Error: Unknown variable: '::osfamily'. (file: /home/runner/work/puppetlabs-firewall/puppetlabs-firewall/spec/fixtures/modules/firewall/manifests/params.pp, line: 7, column: 8) on node fv-az191-465.ggaq42fduhvefhuza3ifwajfkf.bx.internal.cloudapp.net

edit: figured out where osfamily was missing, added it

Copy link
Contributor

@LukasAud LukasAud left a comment

Choose a reason for hiding this comment

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

LGTM

@LukasAud LukasAud merged commit a597671 into puppetlabs:main Jan 27, 2023
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.

7 participants