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

(MODULES-6923) remove staging module #1115

Merged
merged 1 commit into from Oct 18, 2018

Conversation

tphoney
Copy link
Contributor

@tphoney tphoney commented Aug 29, 2018

No description provided.

@tphoney tphoney changed the title (MODULES-6923) replace staging module with archive {WIP}(MODULES-6923) replace staging module with archive Aug 29, 2018
@tphoney tphoney force-pushed the remove_staging branch 2 times, most recently from b29bad9 to dac8e2d Compare August 29, 2018 14:33
@@ -17,6 +17,7 @@
$environment = undef, # environment for staging::file
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter is now unused

@@ -1,17 +1,12 @@
require 'spec_helper'

describe 'mysql::server::mysqltuner' do
on_supported_os.each do |os, facts|
on_supported_os.each do |os, _facts|
Copy link
Contributor

Choose a reason for hiding this comment

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

If no fact are set, does it actually matter to test it on every OS? You're effectively testing the same thing multiple times.

@tphoney
Copy link
Contributor Author

tphoney commented Aug 29, 2018

Many thanks @ekohl , changes incoming

) {

$tuner_location = '/usr/local/bin/mysqltuner'
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to prefer something like this as a class parameter. This gives some more flexibility to users

) {

$tuner_location = '/usr/local/bin/mysqltuner'
if $source {
$_version = $source
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable appears to be unused now and can be removed.

) {

$tuner_location = '/usr/local/bin/mysqltuner'
if $source {
$_version = $source
$_source = $source
} else {
$_version = $version
$_source = "https://github.com/major/MySQLTuner-perl/raw/${version}/mysqltuner.pl"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but GH also has a raw.github.com that's more efficient for them. Might be worth looking into but doesn't have to be part of this PR.

if $source {
$_version = $source
$_source = $source
} else {
$_version = $version
$_source = "https://github.com/major/MySQLTuner-perl/raw/${version}/mysqltuner.pl"
}

if $ensure == 'present' {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the archive now also has an ensure, does it make sense to drop the if and simply have 2 resources which accept $ensure?

Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

An even crazier idea: puppet itself can also download HTTP files so wouldn't this work as well?

file { $tuner_location:
  ensure => $ensure,
  mode   => '0550',
  source => $_source,
}

@tphoney
Copy link
Contributor Author

tphoney commented Aug 29, 2018

@ekohl lovely idea, testing it now.

) {

if $source {
$_version = $source
Copy link
Contributor

Choose a reason for hiding this comment

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

$_version is still unused.

@tphoney
Copy link
Contributor Author

tphoney commented Aug 30, 2018

@ekohl hopefully closer ?

@ekohl
Copy link
Contributor

ekohl commented Aug 30, 2018

Looks good! Bonus points for data types on parameters but yay for the big simplification already :)

{
"name": "puppetlabs/stdlib",
"version_requirement": ">= 3.2.0 < 5.0.0"
},
{
"name": "puppet/staging",
"version_requirement": ">= 1.0.1 < 4.0.0"
"name": "puppetlabs/translate",
Copy link
Collaborator

Choose a reason for hiding this comment

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

the diff would be a tiny bit cleaner if this would still be on top of stdlib, but it doesn't really matter.

file { $tuner_location:
ensure => $ensure,
mode => '0550',
source => $_source,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. I expected a really complicated rewrite by replacing staging with archive, but I also forgot that the file resource can now take https links <3. Since which puppet version is this supported? Does the support puppet version range within this module needs to be adjusted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Double checked this before merging, but http support for file source was in 4.7 which is the lower boundary of the module: https://puppet.com/docs/puppet/4.7/type.html#file-attribute-source , so we should be good without bumping the puppet version.

Copy link
Collaborator

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

commit/PR title needs updating since 'archive' isn't actually used in the end.

@ekohl
Copy link
Contributor

ekohl commented Sep 27, 2018

It'd be nice if this could get merged.

@tphoney tphoney changed the title {WIP}(MODULES-6923) replace staging module with archive (MODULES-6923) remove staging module Sep 27, 2018
@tphoney
Copy link
Contributor Author

tphoney commented Sep 27, 2018

Hi @ekohl and @alexjfisher i changed the commit and pr message. This will get merged once we have got the puppet 6 release for this module done. It will be a major version bump ie X.y.z because of it being backwards incompatible.

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

5 participants