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

Stricter data types #328

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Stricter data types #328

merged 1 commit into from
Nov 26, 2019

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Nov 13, 2019

This sets data types for most parameters that previously had none. It also moves static defaults from params.pp to inline. While this duplicates a few between Redis and the sentinel, the user would already need to know these were linked. The benefit is a much more readable reference documentation.

Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

I don't understand why it is flagged backwards-incompatible.
Just someone that use knif like a fork will have incompatibility.

@ekohl
Copy link
Member Author

ekohl commented Nov 13, 2019

I don't understand why it is flagged backwards-incompatible.
Just someone that use knif like a fork will have incompatibility.

There are changes. Often they're (much) stricter and users can run into this. Where strings used to be accepted, now only integers are. Given the tests are also failing on this indicates I may have been a bit too aggressive in some places.

@ekohl
Copy link
Member Author

ekohl commented Nov 13, 2019

Note to self: $hash_max_ziplist_entries and $hash_max_ziplist_value are only relevant on 3.0. Looks like Ubuntu 16.04 is the only platform that ships it out of the box.

@igalic
Copy link

igalic commented Nov 14, 2019

Can someone explain to me what '__VALUE__' is — or was, in this code?

@ekohl
Copy link
Member Author

ekohl commented Nov 14, 2019

My guess is that it was a clear placeholder for easy testing. Now that this is green I plan to go over every config item to see if they're still correct. I already found 2 parameters that no longer exist in 3.2+. That makes me think about dropping Ubuntu 16.04 which is the last that ships Redis 3.0.

@igalic
Copy link

igalic commented Nov 14, 2019

when will Canonical drop 16.04?

@ekohl
Copy link
Member Author

ekohl commented Nov 14, 2019

Support stops in April 2021, but I want to add a note that it can work if you install the PPA so you have a version newer than 3.0. Maybe we can set manage_repo to true by default on 16.04.

manifests/init.pp Outdated Show resolved Hide resolved
Copy link
Contributor

@dom-nie dom-nie left a comment

Choose a reason for hiding this comment

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

@ekohl I looked at most of variables you are touching and these could use some more tightening and safer limits on types.

manifests/sentinel.pp Outdated Show resolved Hide resolved
manifests/sentinel.pp Outdated Show resolved Hide resolved
manifests/sentinel.pp Outdated Show resolved Hide resolved
manifests/sentinel.pp Outdated Show resolved Hide resolved
manifests/sentinel.pp Outdated Show resolved Hide resolved
manifests/sentinel.pp Outdated Show resolved Hide resolved
manifests/administration.pp Outdated Show resolved Hide resolved
manifests/instance.pp Outdated Show resolved Hide resolved
manifests/instance.pp Outdated Show resolved Hide resolved
manifests/instance.pp Outdated Show resolved Hide resolved
@ekohl
Copy link
Member Author

ekohl commented Nov 18, 2019

Awesome @dom-nie! Going over the parameters was on my TODO list for today and I highly appreciate it.

This sets data types for most parameters that previously had none. It
also moves static defaults from params.pp to inline. While this
duplicates a few between Redis and the sentinel, the user would already
need to know these were linked. The benefit is a much more readable
reference documentation.
@ekohl
Copy link
Member Author

ekohl commented Nov 21, 2019

I think this is now ready for a final review. The String entries have been tightened to String[1] as well.

@ekohl ekohl merged commit 5c97e7e into voxpupuli:master Nov 26, 2019
@ekohl ekohl deleted the updated-params branch November 26, 2019 13:03
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