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

Fix linting and add some type_aliases #5

Merged
merged 8 commits into from
Feb 24, 2023
Merged

Conversation

rwaffen
Copy link
Sponsor Member

@rwaffen rwaffen commented Feb 23, 2023

  • Fix linitng od classes and defines
  • Add Types to have shorter class headers

Copy link
Member

@ananace ananace left a comment

Choose a reason for hiding this comment

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

A first glance looks reasonable, though I've got some thoughts on the choice of several of the type names

types/dns_service_address.pp Outdated Show resolved Hide resolved
types/api_addn_names.pp Outdated Show resolved Hide resolved
types/cluster_cidr.pp Outdated Show resolved Hide resolved
types/addn_names.pp Outdated Show resolved Hide resolved
@rwaffen
Copy link
Sponsor Member Author

rwaffen commented Feb 23, 2023

sure, will change the names. just took 'em from the variable names.

@rwaffen
Copy link
Sponsor Member Author

rwaffen commented Feb 23, 2023

uff, there are a lot of rubocop recommendations 😓 but i din't touched this, was it like this before?

@rwaffen rwaffen requested a review from ananace February 23, 2023 16:14
Comment on lines +4 to +5
Stdlib::Fqdn,
Stdlib::IP::Address::Nosubnet,
Copy link
Member

Choose a reason for hiding this comment

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

This feels like Stdlib::Host, but isn't quite that. Perhaps stdlib should start using the non-compat IP addresses instead.

@SimonHoenscheid
Copy link
Sponsor Member

uff, there are a lot of rubocop recommendations 😓 but i din't touched this, was it like this before?

These comes in with the module sync. I was thinking about to take care of these later this week or next week

@ekohl
Copy link
Member

ekohl commented Feb 23, 2023

I'd suggest to disable RuboCop in CI until you fix it. voxpupuli/modulesync_config@89c424e should make it easy. That at least gives you unit tests etc.

@rwaffen
Copy link
Sponsor Member Author

rwaffen commented Feb 24, 2023

@ananace had to change it like this, otherwise i couldn't get the tests running again ¯_(ツ)_/¯
can you say if this is okay?

you recommended this in #5 (comment)

-  Optional[K8s::Tls_altnames] $api_addn_names        = undef,
+  K8s::Tls_altnames $api_addn_names                  = [],

@@ -157,11 +154,7 @@

file { '/etc/sysctl.d/99-k8s.conf':
Copy link
Member

Choose a reason for hiding this comment

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

for future refrence: we should replace this with our sysctl module

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

was thinking of this, but didnt wanted to inflate the pr much more ^^

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I added #7 to keep track of that

@@ -137,10 +137,7 @@

file { '/etc/modules-load.d/k8s':
ensure => $ensure,
Copy link
Member

Choose a reason for hiding this comment

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

for future reference: we should update this with our kmod module.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

#8 is there to keep track

types/cidr.pp Outdated Show resolved Hide resolved
rwaffen and others added 2 commits February 24, 2023 11:13
Co-authored-by: Tim Meusel <tim@bastelfreak.de>
@ananace
Copy link
Member

ananace commented Feb 24, 2023

@rwaffen The comment was more that there were two types with the only difference being that one was Optional, the code as listed here is correct - a.k.a. in this case it should be an empty array

I should probably go through and make sure things are more consistent in the use of Optional and default values

@ananace
Copy link
Member

ananace commented Feb 24, 2023

I've only done some cursory glances as said, but only got a couple of nitpicks with casing and such left, things that don't necessarily need to be part of this PR

@rwaffen rwaffen merged commit 0b39cdb into voxpupuli:master Feb 24, 2023
@rwaffen rwaffen deleted the linting branch February 24, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants