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

Update to voxpupuli-test 5 #763

Merged
merged 7 commits into from
Jun 28, 2022
Merged

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jun 21, 2022

I'm unsure about removing the default value in powerdns since it could be a breaking change and I don't know what Kafo and the installer does with it

@evgeni
Copy link
Member

evgeni commented Jun 21, 2022

I'm unsure about removing the default value in powerdns since it could be a breaking change and I don't know what Kafo and the installer does with it

user could not have a working setup with an empty key, right? which makes me think it's not a breaking change (as it would only break broken setups).

@@ -1,7 +1,6 @@
# Remote Execution SSH user default parameters
Copy link
Member

Choose a reason for hiding this comment

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

this files triggers:

manifests/plugin/remote_execution/ssh_user/params.pp:16:top_scope_facts:WARNING:top scope fact instead of facts hash

@ekohl
Copy link
Member Author

ekohl commented Jun 22, 2022

I'll investigate why it's failing.

@ekohl ekohl marked this pull request as draft June 22, 2022 15:26
@ekohl
Copy link
Member Author

ekohl commented Jun 23, 2022

It looks like on FreeBSD with Facter 4 the ipaddress_$interface fact is missing. I'm not sure if this is a bug in FacterDB's facts or in Facter itself. I'm guessing a regression in Facter.

It does look sort of up to date: https://www.freebsd.org/cgi/ports.cgi?query=foreman-proxy&stype=all now lists version 3.1.2 (updated in April) so I'm hesitant to just drop it. The good news is that the modern fact (networking) is present. I'm going to use this chance to update the code to modern facts instead of using legacy facts.

And locally I also run into the problem that puppetlabs-apt requires net/ftp which creates problems on Ruby 3, but that shouldn't affect CI for now.

@ekohl ekohl force-pushed the voxpupuli-test-5 branch 2 times, most recently from 195f2df to 1e01b66 Compare June 23, 2022 12:47
@ekohl
Copy link
Member Author

ekohl commented Jun 23, 2022

The scope is now larger, I do think some PRs here deserve their own mention in the changelog. Now I just want to see if the tests are green.

@ekohl
Copy link
Member Author

ekohl commented Jun 23, 2022

I split off #764.

An empty string is an invalid value for API key, but there's no sensible
default either.
This was needed back with Puppet 3 but has been redundant since Puppet <
4 support was dropped.
When a parameter has a default, there is no way to set it back to undef.
That means the Optional part is redundant and the data type can be
simplified.
@ekohl ekohl marked this pull request as ready for review June 24, 2022 14:02
@ehelms
Copy link
Member

ehelms commented Jun 27, 2022

I think this needs a rebase now.

The version in plugin.pp is stricter than foreman_proxy::globals but
that's because you could declare an individual plugin with a specific
version while globally it doesn't make sense to set a specific version
number.
This is not a fact, but rather a global variable. Using getvar is the
right way here, like all other variables in this file.
@ekohl
Copy link
Member Author

ekohl commented Jun 28, 2022

It was up to date, but I made a mistake in adding the data types. I think that's fixed now.

@ehelms ehelms merged commit 7343bb5 into theforeman:master Jun 28, 2022
@ekohl ekohl deleted the voxpupuli-test-5 branch June 28, 2022 17:15
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.

4 participants