Skip to content

Conversation

halimwi
Copy link
Contributor

@halimwi halimwi commented Jul 5, 2018

No description provided.

@halimwi halimwi self-assigned this Jul 5, 2018
@halimwi halimwi requested a review from MartyEwings July 5, 2018 07:12
Copy link
Collaborator

@MartyEwings MartyEwings left a comment

Choose a reason for hiding this comment

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

I think the header for the script needs changed to "#!/bin/bash" or this may not work in ubuntu.

In the main body of the script, I would remove the templated comment lines.

In the JSON file i would put the KB information also

The Travis build also has a few checks to complete

But looks good! Thanks

tasks/kb0009.sh Outdated
FACTER_level="$loglevel" FACTER_service="$peservice" puppet apply -e "augeas {'toggle logging level': incl => \"/etc/puppetlabs/$::service/logback.xml\", lens => 'Xml.lns', context => \"/files/etc/puppetlabs/$::service/logback.xml/configuration/root/#attribute\", changes => \"set level \'$::level\'\"}~> service {\"pe-$::service\": ensure => running }"
echo "Updated $peservice log level to $loglevel"

elif [ -e "/etc/default/pe-puppetserver" ] # cover ubuntu
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for differentiating between EL and Ubuntu for this task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartyEwings good catch on that, updated the json and .sh file.

@jarretlavallee There is no difference for this scenario, mainly for future maintainability should we need to add additional commands in which EL and Ubuntu might differ. But for now, I have updated the script. Thanks!

@halimwi
Copy link
Contributor Author

halimwi commented Jul 6, 2018

The Travis CI can't seem to interpret Task assigned variable $PT_xxx

Any idea what to be done to pass this check?

PT_loglevel is referenced but not assigned.

@MartyEwings
Copy link
Collaborator

@halimwi I think this may be because the task declares the env variable in the json file, and travis expects it in the script, ill see if some of the official modules with params work around this in some way.

We may have to ignore this check or write some new rules

@MartyEwings
Copy link
Collaborator

@halimwi from googling i think for best practice if we do:

declare (variable name)

at the start of the script this should leave the contents intact but also clear this error

@MartyEwings
Copy link
Collaborator

@halimwi just to get the full picture on this we can declare in our scripts as we will always know the name of env variable as we set them in the json

also maybe for future reference

Note: This message only triggers for variables with lowercase characters in their name (foo and kFOO but not FOO) due to the standard convention of using lowercase variable names for unexported, local variables.

so for these env variables we could use uppercase to circumvent this

@MartyEwings MartyEwings merged commit 066d1ae into puppetlabs:master Jul 9, 2018
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.

3 participants