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.

Sign up for GitHub

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

Jump to bottom

(MODULES-6923) remove staging module #1115

Merged
merged 1 commit into from
Oct 18, 2018
Merged

(MODULES-6923) remove staging module #1115

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
@tphoney tphoney mentioned this pull request Aug 29, 2018
Closed
ekohl
ekohl reviewed Aug 29, 2018
View reviewed changes
manifests/server/mysqltuner.pp Outdated
@@ -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

spec/classes/mysql_server_mysqltuner_spec.rb Outdated
@@ -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

ekohl
ekohl reviewed Aug 29, 2018
View reviewed changes
manifests/server/mysqltuner.pp Outdated
) {

$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

manifests/server/mysqltuner.pp Outdated
) {

$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.

manifests/server/mysqltuner.pp
) {

$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.

manifests/server/mysqltuner.pp Outdated
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?

ekohl
ekohl reviewed Aug 29, 2018
View reviewed changes
Copy link
Contributor

@ekohl ekohl left a comment •
edited
Loading

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.

ekohl
ekohl reviewed Aug 29, 2018
View reviewed changes
manifests/server/mysqltuner.pp Outdated
) {

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 :)

bastelfreak
bastelfreak reviewed Sep 7, 2018
View reviewed changes
metadata.json
{
"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.

bastelfreak
bastelfreak reviewed Sep 7, 2018
View reviewed changes
manifests/server/mysqltuner.pp
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.

alexjfisher
alexjfisher requested changes Sep 10, 2018
View reviewed changes
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.

@HelenCampbell HelenCampbell merged commit a762731 into puppetlabs:master Oct 18, 2018
@sbadia sbadia mentioned this pull request Nov 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HelenCampbell HelenCampbell HelenCampbell left review comments

@bastelfreak bastelfreak bastelfreak left review comments

@alexjfisher alexjfisher alexjfisher requested changes

@ekohl ekohl ekohl approved these changes

Assignees
No one assigned
Labels
backwards-incompatible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tphoney @ekohl @alexjfisher @bastelfreak @HelenCampbell