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-3593) Changes default MOTD to something more useful #76

Merged
merged 3 commits into from
Aug 11, 2017
Merged

(MODULES-3593) Changes default MOTD to something more useful #76

merged 3 commits into from
Aug 11, 2017

Conversation

8675309
Copy link
Contributor

@8675309 8675309 commented Aug 7, 2017

Improves out-of-the-box useability. Changes default message to include FQDN, Processor, and Kernel in addition to the old messages Operating system and free memory. Removes domain as this is included in FQDN. Removes test to test when domain fact is nil.

Improves out-of-the-box useability. Changes default message to include FQDN, Processor, and Kernel in addition to the old messages Operating system and free memory. Removes domain as this is included in FQDN. Removes test to test when domain fact is nil.
@glennsarti
Copy link
Contributor

glennsarti commented Aug 7, 2017

As the templates/motd.erb file is being modified and this affects the default MOTD on Windows, should there be tests for this too?

@8675309
Copy link
Contributor Author

8675309 commented Aug 9, 2017

@glennsarti this is an interesting comment. We reviewed the tests, and it appears that this wouldn't add valuable coverage. Our template test was modified to take different facts and produce different output. A windows test would be redundant because we would just be changing facts in the stub to have windows facts rather than linux facts.

Windows has it's own conditional in init.pp, however, the template has been rendered and stored in $motd_content at the top of the file. As for the code in the windows conditional, it appears it is being covered in the windows test by passing in a arbitrary string.

I'm happy to review the tests together if you still have concerns. Thanks for your comment!

<%= @operatingsystem %> <%= @operatingsystemrelease %> <%= @architecture %>

FQDN: <%= @fqdn %> (<%= @ipaddress %>)
Processor: <%= @processor1 %>
Copy link
Contributor

@glennsarti glennsarti Aug 10, 2017

Choose a reason for hiding this comment

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

This should probably be processor0 not processor1. On a single physical CPU with multiple cores, only processor0 exists.

@glennsarti
Copy link
Contributor

Yay for tests!!! Thanks!

The procssor fact stores processors in an array, so the first one is at '0' not '1'. Changed templates and tests accordingly.
@glennsarti glennsarti merged commit fd5a42f into puppetlabs:master Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants