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

when certs are symlinks bolt complained, looking for puppet x509 stuff #27

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

prolixalias
Copy link

various typos in README
unused param (port)
follow links to certificate(s)

@prolixalias prolixalias requested a review from a team as a code owner August 9, 2022 21:22
@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.

@MartyEwings MartyEwings added the bug Something isn't working label Aug 23, 2022
@prolixalias prolixalias force-pushed the feature_typos_and_linting branch 2 times, most recently from cd07e65 to 013c5c5 Compare August 31, 2022 00:51
@MartyEwings
Copy link
Contributor

@m0dular can you do a quick review of this? @prolixalias would you beable to rebase over the latest commits Thanks!

prolixalias and others added 5 commits September 30, 2022 10:30
Also remove unnecessary require
unused param (port)

follow links to certificate(s)

add custom type test - dynamic for factset's FQDN

linting
@prolixalias
Copy link
Author

@m0dular can you do a quick review of this? @prolixalias would you beable to rebase over the latest commits Thanks!

Sure thing Marty. Done.

puppet_conf: 'https://github.com/puppetlabs/puppetlabs-puppet_conf'
ruby_task_helper: 'https://[email protected]/puppetlabs/puppetlabs-ruby_task_helper'
stdlib: 'https://github.com/puppetlabs/puppetlabs-stdlib'
deploy_pe: 'https://github.com/jarretlavallee/puppet-deploy_pe'
symlinks:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution, looks good! I only had a question about this symlinks entry, what does this do? I was able to run the unit tests with pdk test unit with and without this, so I think it should set up everything related to this module. Let me know if I'm missing something, I haven't used symlinks in fixtures before so it's entirely possible.

Copy link
Author

Choose a reason for hiding this comment

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

Hello! I was working around needing influxdb_setup type available to: is_expected.to contain_influxdb_setup(Facter.value(:networking)['fqdn']) in spec/classes/init_spec.rb

Starting to wonder if my pdk is/was misconfigured. I couldn't get it to pass without symlink to itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

usually puppetlabs_spec_helper creates the symlink, thats why we removed the symlink option from a lot of modules 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Shall I resubmit without the additional symlink, not to be confused with title of PR. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be great, if you could also update the title to something like "Follow symlinks and fix spelling errors" then I think we're good to merge.

@m0dular
Copy link
Contributor

m0dular commented Oct 7, 2022

I think you helped expose an issue where the manifest isn't passing the port to the influxdb_setup resource, thanks for finding that and removing it since it's unused. I've created an issue where we should allow for customizing this.

@prolixalias
Copy link
Author

prolixalias commented Oct 11, 2022

I think you helped expose an issue where the manifest isn't passing the port to the influxdb_setup resource, thanks for finding that and removing it since it's unused. I've created an issue where we should allow for customizing this.

I'll take #53 if that's okay... Let's get #27 through, will start on port customization tonight off-hours

@m0dular
Copy link
Contributor

m0dular commented Feb 8, 2023

Sorry for not getting back on this sooner. I think since #53 has been fixed and the linting and typos got fixed somewhere along the way, the only thing left here should be following symlinks. If you could either rebase this PR, or just close and open a new one we should be able to merge that.

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants