Rewrite updated_ood_portal to be Ruby

[treydock]

Should resolve #100.

This changes checksum to be based on non-comments and also replacing simple cmp with diff that ignores comments.

Just FYI the way OSC deploys ood-portal.conf will NOT ignore comments because we generate /etc/ood/config/ood-portal.conf not using update_ood_portal and then we use Puppet to put the file into correct location and no way for it to ignore comments.

One flaw with this approach is changes to the comments won't even get written to as .new file. If I keep cmp instead of using diff then get something like this when comments change:

/opt/ood/ood-portal-generator/sbin/update_ood_portal 
Generating Apache config using YAML config: '/etc/ood/config/ood_portal.yml'
WARNING: Checksum of /opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf does not match previous value, not replacing.
Generating new Apache config at: '/opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf.new'

[root@53cfead15bf4 ondemand]# diff -u /opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf /opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf.new
--- /opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf	2019-11-07 17:41:58.487211000 +0000
+++ /opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf.new	2019-11-07 17:42:59.970782000 +0000
@@ -31,6 +31,12 @@
 #      sudo service httpd24-httpd condrestart
 #      sudo service httpd24-htcacheclean condrestart
 #
+#      # For CentOS 7
+#      sudo systemctl try-restart httpd24-httpd.service httpd24-htcacheclean.service
+#
+#      # For CentOS 8
+#      sudo systemctl try-restart httpd.service htcacheclean.service
+#

[ericfranz]

How does this achieve the goal?

The checksum is used to verify that the administrator has not hand-modified the ood-portal.conf Apache config. If the administrator never hand-modifies the config, the checksum will ensure that REPLACE=true every time this script is run, regardless of whether or not the checksum is of the ood-portal.conf or of ood-portal.conf without comments.

Note: because the checksum file is being made of ood-portal.conf without comments, but then is being used to check a file ood-portal.conf that contains comments, won't this new checksum always fail? So REPLACE=false will now always be the case.

Regardless, since we are now sticking with if ! cmp -s "${NEW_APACHE}" "${APACHE}" and the comparison is not excluding differences in comments, if somehow REPLACE=true the Apache config would get replaced and Apache would be restarted.

[ericfranz]

Also, https://github.com/OSC/ondemand/blob/master/ood-portal-generator/ is getting more complex, and I wonder if it makes more sense to move some of this logic into the Ruby code itself. Or at least, write some minitest tests that exercise this script with different inputs and outputs to verify the desired functionality. Happy to help with test scaffolding.

[treydock]

@ericfranz I agree, this is getting complex enough this should be in Ruby. I am working on moving update_ood_portal to Ruby code and will use RSpec because I'm more familiar with RSpec than other testing tools for Ruby.

[treydock]

Completely refactored update_ood_portal to be Ruby.

Example output:

[root@58a65dd0c11b /]# /opt/ood/ood-portal-generator/sbin/update_ood_portal --detailed-exitcode 
Generating Apache config using YAML config: '/etc/ood/config/ood_portal.yml'
WARNING: Checksum of /opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf does not match previous value, not replacing.
Generating new Apache config at: '/opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf.new'
[root@58a65dd0c11b /]# echo $?
4
[root@58a65dd0c11b /]# /opt/ood/ood-portal-generator/sbin/update_ood_portal --detailed-exitcode -f
Generating Apache config using YAML config: '/etc/ood/config/ood_portal.yml'
Backing up previous Apache config to: '/opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf.20191108T174136'
Generating new Apache config at: '/opt/rh/httpd24/root/etc/httpd/conf.d/ood-portal.conf'
Generating Apache config checksum file: '/etc/ood/config/ood_portal.sha256sum'
Completed successfully!

Restart the httpd24-httpd service now.

Suggested command:
    sudo systemctl try-restart httpd24-httpd.service httpd24-htcacheclean.service

[root@58a65dd0c11b /]# echo $?
3

[ericfranz]

def scl_apache?

[treydock]

Changed made.

[ericfranz]

def sum_path for clarity?

[treydock]

Change made.

[ericfranz]

Added comments but it was all minor code quality suggestions or test improvements. So if you are happy and/or don't have time to make those changes I say just merge and we can make notes to improve later.

[treydock]

The changes are simple except the one for tests. I'll make them Tuesday at latest. The unit test either has to mock because can't assume write access to like /opt and /etc or has to be run as system tests in a container. For unit I have to at least mock file paths with Tempfile to avoid writing to inaccessible places but system tests could better stress this inside docker where we have root.

[ericfranz]

I didn't communicate my idea for the tests well. What I was thinking was a set of tests that exercised the outside boundary of the script, without mocking any parts inside of the script. If all of the system paths can be modified using env vars (CONFIG, SUM, APACHE) the set of tests could use https://github.com/thoughtbot/climate_control and follow the pattern:

it 'does not replace if no changes detected' do
  # 1. copy fixtures to tmp so that sum, apache, and config are in a tmpdir structure
  # 2. use climate control to set CONFIG, SUM, APACHE in block
  # 3. execute OodPortalGenerator::Application.start("update_ood_portal") with the CLI arguments
  # 4. check exit status
end

[ericfranz]

Following up on the idea I posed with using climate control, you would also add argv as an argument to start i.e. start(mode, argv=ARGV) and then in the test you could do

OodPortalGenerator::Application.start("update_ood_portal", ["--detailed-exitcodes"])

[treydock]

@ericfranz Look now. I added spec/update_ood_portal_spec.rb that only changes environment variables to point at fixtures using Tempfile paths. Avoiding the function that calls exit! is ideal for other spec tests because if something goes wrong and exit! is called due to an exception caught in start then the rest of the tests are skipped. The only place this can happen now is in spec/update_ood_portal_spec.rb.

[treydock]

Added Rake tasks to aid in running tests from root of this project without ugly "cd" type Travis scripts

[treydock]

Rewrote travis.yml to use stages so that unit tests happen before system tests and only running tests on master, tags and pull requests to avoid double testing during pull requests when the branch is on this repo.

[treydock]

Looks like getting this in Travis sh: 1: source: not found. I will investigate.

[treydock]

Looks like source was not present in Travis because default shell is not /bin/bash. To avoid this issue when people run ood-portal-generator I moved the environment parsing of /etc/os-release to use dotenv.

[ericfranz]

@treydock I'm happy with the changes. Ready to merge or did you want to do anything else on this first?

[treydock]

@ericfranz This PR is ready to merge.