This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Expose GOVUK_DATA_SYNC_PERIOD ENV variable for govuk_app_config #10770

Merged

ChrisBAshton merged 2 commits into master from GOVUK_DATA_SYNC_PERIOD

Oct 20, 2020

Contributor

ChrisBAshton commented Oct 16, 2020

This is to enable alphagov/govuk_app_config#160,
which ignores certain exceptions if they occur during the data
sync period. We want to set this via Puppet so that it is DRY and
configured in only one place.

Trello: https://trello.com/c/rAlvGlSp/2123-5-ignore-data-sync-errors-in-govukappconfig

ChrisBAshton force-pushed the GOVUK_DATA_SYNC_PERIOD branch 15 times, most recently from e6da4e4 to c8573ac Compare

October 16, 2020 16:16


          Expose GOVUK_DATA_SYNC_PERIOD ENV variable for govuk_app_config

1dcf035

This is to enable alphagov/govuk_app_config#160,
which ignores certain exceptions if they occur during the data
sync period. We want to set this via Puppet so that it is
configured in only one repo. In the next commit I will DRY up the
values so that they're only set in one place, but for now, there
is precedent for duplicating the values - see
/~https://github.com/alphagov/govuk-puppet/blob/5b1dc13ea3c7180318da284fe1dbe90a14460234/modules/monitoring/manifests/contacts.pp#L57-L59

We only want to define GOVUK_DATA_SYNC_PERIOD on Integration and
Staging. If we hit an error on Production during the data sync
period, we still want to log it to Sentry.

Trello: https://trello.com/c/rAlvGlSp/2123-5-ignore-data-sync-errors-in-govukappconfig

ChrisBAshton force-pushed the GOVUK_DATA_SYNC_PERIOD branch from c8573ac to 198d072 Compare

October 19, 2020 09:34

kevindew reviewed

View reviewed changes

Member

kevindew left a comment

This looks neat Chris. I've a few minor comments on the function but approach seems sound.

modules/govuk/lib/puppet/parser/functions/data_sync_times.rb Outdated Show resolved Hide resolved

modules/govuk/lib/puppet/parser/functions/data_sync_times.rb

+              module Puppet::Parser::Functions
+                newfunction(:data_sync_times, :type => :rvalue, :doc => <<-EOS
+              Return information about the data sync times, depending on the argument passed
+                  EOS

Member

kevindew Oct 19, 2020

Ooh this is exotic, I've never seen a puppet function definition before. It seems a neat solution to the problem 👍

Contributor Author

ChrisBAshton Oct 19, 2020

I took inspiration from other functions we have in govuk-puppet, after initially trying (and failing) to define the start/finish times in YAML. Could have pursued YAML further but as we sometimes need the raw values and sometimes need a representation of them, seemed just as clean to put them in here. 👍

modules/govuk/lib/puppet/parser/functions/data_sync_times.rb

		@@ -0,0 +1,38 @@
		module Puppet::Parser::Functions
		newfunction(:data_sync_times, :type => :rvalue, :doc => <<-EOS

Member

kevindew Oct 19, 2020

I wonder if you want to name this as format_data_sync_time since the primary role of the function is the formatting?

Contributor

AlanGabbianelli Oct 19, 2020 •

edited

Loading

Since the function returns values and doesn't change anything, I'd probably avoid the use of a verb like "format" in its name. Calling it format_data_sync_time makes it sound like it's either editing something somewhere or returning edited versions of passed arguments. I think data_sync_times is accurate for what it does: returning appropriate data sync time values based on the argument passed.

Contributor Author

ChrisBAshton Oct 20, 2020

I think I agree with Alan here 👍

modules/govuk/lib/puppet/parser/functions/data_sync_times.rb Outdated Show resolved Hide resolved

ChrisBAshton force-pushed the GOVUK_DATA_SYNC_PERIOD branch 2 times, most recently from f6daa94 to 6f9fbd8 Compare

October 19, 2020 13:49

AlanGabbianelli approved these changes

View reviewed changes

Contributor

AlanGabbianelli left a comment

Looks good, I just left a small suggestion (not a blocker!)

modules/govuk/lib/puppet/parser/functions/data_sync_times.rb Outdated Show resolved Hide resolved


          DRY up the data sync start/end times

45fa8c2

These values are beginning to leak into multiple places. It is good
maintenance practice to try to keep them in one place.

There is probably a cleaner way of doing this with YML config, but
given that some places need the start/end times and others need
specifically the start/end hour and minute, a function seems a
reasonable thing to create to encapsulate all that knowledge.

In any case, it is an improvement on trying to duplicate the
values throughout the repository.

ChrisBAshton force-pushed the GOVUK_DATA_SYNC_PERIOD branch from a6681b3 to 45fa8c2 Compare

October 20, 2020 07:12

ChrisBAshton merged commit 1a69ab2 into master

ChrisBAshton deleted the GOVUK_DATA_SYNC_PERIOD branch

October 20, 2020 07:51

ChrisBAshton commented

View reviewed changes

modules/govuk/manifests/deploy/config.pp

@@ @@ -127,6 +127,12 @@ @@
                   'GOVUK_CSP_REPORT_URI': value      => $csp_report_uri;
                 }
+                if $::aws_migration and ($::aws_environment != 'production') {
+                  govuk_envvar {
+                    'GOVUK_DATA_SYNC_PERIOD': value => data_sync_times('time_range');

Contributor Author

ChrisBAshton Oct 20, 2020

So this does seem to be setting it (don't worry about the lack of leading zeros - Time.parse still parses them fine):

$ more /etc/govuk/env.d/GOVUK_DATA_SYNC_PERIOD
22:0-8:0

But if I echo $GOVUK_DATA_SYNC_PERIOD I get nothing. Do we know how/whether these variables get exported as traditional ENV vars @kevindew ?

Member

kevindew Oct 20, 2020

I'm not sure exactly how they work - I think the govuk_setenv command sets them:

kevindew@ec2-integration-blue-email_alert_api-ip-10-1-6-234:~$ govuk_setenv email-alert-api env | grep GOVUK_DATA_SYNC_PERIOD
GOVUK_DATA_SYNC_PERIOD=22:0-8:0

You can see the env var if you go into an app console. For example Email Alert API on integration:

irb(main):002:0> ENV["GOVUK_DATA_SYNC_PERIOD"]
=> "22:0-8:0"

Contributor Author

ChrisBAshton Oct 20, 2020

That's a relief - thanks for confirming!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet