Merge lp:~suligap/canonical-identity-provider/charm-de-oops into lp:~ubuntuone-pqm-team/canonical-identity-provider/charm

[Przemysław Suliga]

Remove OOPS config

"Remove OOPSes"
https://code.launchpad.net/~suligap/canonical-identity-provider/+git/canonical-identity-provider/+merge/385205
depends on this change, since the charm currently assumes that settings.OOPSES
(removed in the above MP) exists in settings_base.py

[Daniel Manrique (roadmr)]

+1 code-wise, also worth double-checking with GUillermo in case there's anything we need to be careful with, with removing config options from charms.

The reason is that the CI tests for SSO will try to set the configuration for both the old and new revision, and if things are not updated in the correct order, CI might fail with a version of the charm that doesn't support a specific option, and that'll leave us without a testing path and might require manual intervention.

review: Approve

[Guillermo Gonzalez (verterok)]

If I remember correctly we need multiple MP to remove a config in a "safe" way.

1) remove the usage in templates and charm code, but keep it in config.yaml

2) land and deploy the changes from (1)

3) remove the config entries from local.yaml and mojo spec deployer configs

4) land that change

5) remove configs from config.yaml

Might be exploding the thing a bit too much and some steps could be merged, need to double check the mojo spec and CI to be sure

[Przemysław Suliga (suligap)]

Thanks!

FWIW, here's the spec MP: https://code.launchpad.net/~suligap/canonical-mojo-specs/mojo-ols-canonical-identity-provider-de-oops/+merge/385258.

I wonder if it's enough to just do:

1. Land the spec, local.yaml changes.
2. Land this MP.

Since in this charm we only use these config options in templates and unit tests? (and that's what's being removed here apart from the config options themselves.) I'm probably missing something.

[Przemysław Suliga (suligap)]

Update: related spec change is https://code.launchpad.net/~suligap/canonical-mojo-specs/mojo-ols-canonical-identity-provider-de-oops-updated/+merge/415455

[Guillermo Gonzalez (verterok)]

+1

review: Approve

=== modified file 'config.yaml'
--- config.yaml	2019-09-26 20:45:23 +0000
+++ config.yaml	2020-06-08 09:08:44 +0000
@@ -41,22 +41,6 @@
         type: string
         default: ""
         description: Django secret key. Service will not run without this set.
-    oops_config:
-        type: string
-        default: ""
-        description: >
-          A python dict representing an amqp oops_publisher, with the followinga
-          fields; inherit_id, exchange_name, routing_key, vhost, host user,
-          password
-        content_type: python
-    oops_password:
-        type: string
-        default: ""
-        description: "secret for oops_config password section"
-    oops_reporter:
-        default: AS-PRODUCTION.SSO
-        type: string
-        description: ""
     captcha_private_key:
         type: string
         default: ""

=== modified file 'deploy.yaml'
--- deploy.yaml	2019-08-15 21:49:05 +0000
+++ deploy.yaml	2020-06-08 09:08:44 +0000
@@ -5,7 +5,6 @@
       charm: local:xenial/canonical-identity-provider
       options:
         deployment: devel
-        oops_reporter: AS-DEVEL.SSO
         secret_key: development
         statsd_prefix: dev.sso
         db_user: sso

=== modified file 'playbook.yaml'
--- playbook.yaml	2020-06-03 06:43:21 +0000
+++ playbook.yaml	2020-06-08 09:08:44 +0000
@@ -12,7 +12,6 @@
     python: "{{ venv }}/bin/python"
     wheel_dir: "{{ current_dir }}/branches/wheels"
     logs_dir: "{{ basedir }}/logs"
-    oops_dir: "{{ basedir }}/logs/www-oops"
     conf_dir: "{{ basedir }}/etc"
     run_dir: "{{ basedir }}/run"
     bin_dir: "{{ basedir }}/bin"
@@ -55,7 +54,6 @@
       writable_dirs:
         - "{{ logs_dir }}"
         - "{{ run_dir }}"
-        - "{{ oops_dir }}"
         - "{{ migrate_log_dir }}"
 
     - role: payload

=== modified file 'templates/settings.py.j2'
--- templates/settings.py.j2	2019-09-26 20:45:23 +0000
+++ templates/settings.py.j2	2020-06-08 09:08:44 +0000
@@ -89,14 +89,6 @@
 {% if noreply_from_address %}
 NOREPLY_FROM_ADDRESS = "{{ noreply_from_address }}"
 {% endif %}
-{% if oops_config %}
-{# TODO: ensure oops_config is dict? #}
-OOPSES['publishers'].append({{ oops_config }})
-{% if oops_password %}
-OOPSES['publishers'][-1]['password'] = "{{ oops_password }}"
-{% endif %}
-{% endif %}
-OOPSES['template']['reporter'] = '{{ oops_reporter }}'
 OPENID_LAUNCHPAD_STAFF_TEAMS = {{ openid_launchpad_staff_teams }}
 OPENID_LAUNCHPAD_TEAMS_MAPPING = {{ openid_launchpad_teams_mapping }}
 {% if openid_token_limit %}

=== modified file 'unit_tests/test_templates.py'
--- unit_tests/test_templates.py	2019-05-22 17:28:30 +0000
+++ unit_tests/test_templates.py	2020-06-08 09:08:44 +0000
@@ -65,7 +65,6 @@
         # mock things in settings_base module that we modify, rather than
         # overwrite
         m = imp.new_module('settings_base')
-        m.OOPSES = {'publishers': [], 'template': {}}
         m.DATABASES = {'default': {}}
         m.CACHES = {'default': {}}
         m.SAML2IDP_CONFIG = {}
@@ -167,15 +166,6 @@
         self.assertEqual(DB['HOST'], expected_config['db_host'])
         self.assertEqual(DB['PORT'], expected_config['db_port'])
 
-        if expected_config.get('oops_config', "") != "":
-            expected = {}
-            expected.update(ast.literal_eval(expected_config['oops_config']))
-            if expected_config.get('oops_password', "") != "":
-                expected['password'] = expected_config['oops_password']
-            self.assertEqual(settings.OOPSES['publishers'][0], expected)
-        else:
-            self.assertEqual(len(settings.OOPSES['publishers']), 0)
-
         if expected_config.get('statsd_hostport', "") != "":
             host, port = expected_config['statsd_hostport'].split(':')
             self.assertEqual(settings.STATSD_HOST, host)
@@ -220,12 +210,6 @@
         settings = self.do_render(self.context)
         self.assert_settings(settings)
 
-    def test_oops_config(self):
-        oops = '{"a": "b"}'
-        self.context['oops_config'] = oops
-        settings = self.do_render(self.context)
-        self.assert_settings(settings, oops_config=oops)
-
     def test_statds_hostport(self):
         hostport = "localhost:777"
         self.context['statsd_hostport'] = hostport