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 default consul version 1.2.3->1.16.1 #637

Merged
merged 1 commit into from Oct 31, 2023

Conversation

bastelfreak
Copy link
Member

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

@@ -117,51 +117,7 @@ class { 'consul':
end

describe command('consul version') do
its(:stdout) { is_expected.to match %r{Consul v1.2.0} }
Copy link
Member Author

Choose a reason for hiding this comment

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

the whole block was a duplicate, we don't need to test it multiple times.

@bastelfreak bastelfreak force-pushed the version branch 2 times, most recently from b099ac6 to 05fb6ce Compare August 21, 2023 19:52
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

See in-line comment, but if not appropriate with this module (I am not a user of consul) I am fine with this.

String[1] $version = '1.2.3',
String[1] $version = '1.16.1',
Copy link
Member

Choose a reason for hiding this comment

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

I generally wonder if it is not better to not set any default version, and mandate the user to explicitly put what they want. This mean that we have to set an arbitrary version for testing, but we do not have to live with having to choose to default to an old-legacy-deprecated version after a few years or break backwards compatibility when a new version is available.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that and I don't know if I like it or not. We need to do a major release anyway so I thought it's a good time to bump the version, also consul supports upgrades between versions. I also doubt that most users are still on 1.2.3 because that's really really really old and not supported anymore.

@smortex smortex mentioned this pull request Oct 31, 2023
@bastelfreak bastelfreak merged commit 8601c6f into voxpupuli:master Oct 31, 2023
16 checks passed
@bastelfreak bastelfreak deleted the version branch October 31, 2023 07:41
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.

None yet

2 participants