Repository: mercadona/rele
Pull request #178 (2020-07-17 10:15:23)

Deprecate GC_PROJECT_ID setting
### :tophat: What?

Deprecate GC_PROJECT_ID. I was even thinking about removing it. Since we can fallback to using the credentials, the removal shouldnt be a breaking change. What do you all think? 

### :thinking: Why?

Simpler setup or a user. Less settings == less hassle

### :link: Related issue

#176 



Patch:
diff --git a/docs/reference.rst b/docs/reference.rst
index 863bfdb..ffcd7d9 100644
--- a/docs/reference.rst
+++ b/docs/reference.rst
@@ -5,8 +5,8 @@ API Reference
 
 .. _ client
 
-Client
-------
+Clients
+-------
 
 .. autoclass:: rele.client.Publisher
    :members:
@@ -14,6 +14,12 @@ Client
 
 .. _ publish
 
+.. autoclass:: rele.client.Subscriber
+   :members:
+
+
+.. _ subscriber
+
 Publish
 -------
 
diff --git a/docs/settings.rst b/docs/settings.rst
index 63920d9..6af1113 100644
--- a/docs/settings.rst
+++ b/docs/settings.rst
@@ -40,13 +40,20 @@ Example::
 
 Path to service account json file with access to PubSub
 
+
+.. _settings_project_id:
+
 ``GC_PROJECT_ID``
 ------------------
 
-**Required**
+**Deprecated**
 
 Valid Google Project ID
 
+.. warning:: This will be removed in an upcoming release in favor of getting
+    project_id from the credentials.
+
+
 ``MIDDLEWARE``
 ------------------
 
diff --git a/rele/client.py b/rele/client.py
index cc39b92..8ba1be1 100644
--- a/rele/client.py
+++ b/rele/client.py
@@ -32,24 +32,19 @@ class Subscriber:
     For convenience, this class wraps the creation and consumption of a topic
     subscription.
 
-    :param gc_project_id: string Google Cloud Project ID.
-    :param credentials: string Google Cloud Credentials.
+    :param gc_project_id: str :ref:`settings_project_id` .
+    :param credentials: obj :meth:`~rele.config.Config.credentials`.
     :param default_ack_deadline: int Ack Deadline defined in settings
     """
 
-    def __init__(self, gc_project_id=None, credentials=None, default_ack_deadline=None):
-
-        if gc_project_id is None or credentials is None:
-            creds, project = get_google_defaults()
-
-        self._gc_project_id = gc_project_id or project
+    def __init__(self, gc_project_id, credentials, default_ack_deadline=None):
+        self._gc_project_id = gc_project_id
         self._ack_deadline = default_ack_deadline or DEFAULT_ACK_DEADLINE
-        _credentials = credentials or creds
 
         if USE_EMULATOR:
             self._client = pubsub_v1.SubscriberClient()
         else:
-            self._client = pubsub_v1.SubscriberClient(credentials=_credentials)
+            self._client = pubsub_v1.SubscriberClient(credentials=credentials)
 
     def create_subscription(self, subscription, topic):
         """Handles creating the subscription when it does not exists.
@@ -82,7 +77,7 @@ class Subscriber:
 
         :param subscription_name: str Subscription name
         :param callback: Function which act on a topic message
-        :param scheduler: `Thread pool-based scheduler.<https://googleapis.dev/python/pubsub/latest/subscriber/api/scheduler.html?highlight=threadscheduler#google.cloud.pubsub_v1.subscriber.scheduler.ThreadScheduler>`_  # noqa
+        :param scheduler: `Thread pool-based scheduler. <https://googleapis.dev/python/pubsub/latest/subscriber/api/scheduler.html?highlight=threadscheduler#google.cloud.pubsub_v1.subscriber.scheduler.ThreadScheduler>`_  # noqa
         :return: `Future <https://googleapis.github.io/google-cloud-python/latest/pubsub/subscriber/api/futures.html>`_  # noqa
         """
         subscription_path = self._client.subscription_path(
diff --git a/rele/config.py b/rele/config.py
index 8dddf5c..5015ff1 100644
--- a/rele/config.py
+++ b/rele/config.py
@@ -1,5 +1,6 @@
 import importlib
 import os
+import warnings
 
 from google.oauth2 import service_account
 
@@ -21,10 +22,7 @@ class Config:
     """
 
     def __init__(self, setting):
-        if setting.get("GC_PROJECT_ID") is None:
-            credentials, project = get_google_defaults()
-
-        self.gc_project_id = setting.get("GC_PROJECT_ID") or project
+        self._project_id = setting.get("GC_PROJECT_ID")
         self.gc_credentials_path = setting.get("GC_CREDENTIALS_PATH")
         self.app_name = setting.get("APP_NAME")
         self.sub_prefix = setting.get("SUB_PREFIX")
@@ -37,6 +35,7 @@ class Config:
         self.publisher_timeout = setting.get("PUBLISHER_TIMEOUT", 3.0)
         self.threads_per_subscription = setting.get("THREADS_PER_SUBSCRIPTION", 2)
         self.filter_by = setting.get("FILTER_SUBS_BY")
+        self._credentials = None
 
     @property
     def encoder(self):
@@ -47,12 +46,25 @@ class Config:
     @property
     def credentials(self):
         if self.gc_credentials_path:
-            return service_account.Credentials.from_service_account_file(
+            self._credentials = service_account.Credentials.from_service_account_file(
                 self.gc_credentials_path
             )
         else:
-            credentials, project_id = get_google_defaults()
-            return credentials
+            credentials, __ = get_google_defaults()
+            self._credentials = credentials
+        return self._credentials
+
+    @property
+    def gc_project_id(self):
+        if self._project_id:
+            warnings.warn(
+                "GC_PROJECT_ID is deprecated in a future release.", DeprecationWarning
+            )
+            return self._project_id
+        elif self.credentials:
+            return self.credentials.project_id
+        else:
+            return None
 
 
 def setup(setting=None, **kwargs):


Test patch:
diff --git a/tests/commands/test_runrele.py b/tests/commands/test_runrele.py
index fac1554..84671c2 100644
--- a/tests/commands/test_runrele.py
+++ b/tests/commands/test_runrele.py
@@ -9,7 +9,9 @@ from rele import Worker
 class TestRunReleCommand:
     @pytest.fixture(autouse=True)
     def worker_wait_forever(self):
-        with patch.object(Worker, "_wait_forever", return_value=None) as p:
+        with patch.object(
+            Worker, "_wait_forever", return_value=None, autospec=True
+        ) as p:
             yield p
 
     @pytest.fixture
diff --git a/tests/conftest.py b/tests/conftest.py
index 7706898..120493d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -24,7 +24,6 @@ def config(project_id):
         {
             "APP_NAME": "rele",
             "SUB_PREFIX": "rele",
-            "GC_PROJECT_ID": project_id,
             "GC_CREDENTIALS_PATH": "tests/dummy-pub-sub-credentials.json",
             "MIDDLEWARE": ["rele.contrib.LoggingMiddleware"],
         }
@@ -33,7 +32,7 @@ def config(project_id):
 
 @pytest.fixture
 def subscriber(project_id, config):
-    return Subscriber(project_id, config.credentials, 60)
+    return Subscriber(config.gc_project_id, config.credentials, 60)
 
 
 @pytest.fixture
diff --git a/tests/test_client.py b/tests/test_client.py
index 1325a83..86a07d2 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -140,6 +140,7 @@ class TestSubscriber:
         _mocked_client.assert_called_once_with(
             ack_deadline_seconds=60, name=expected_subscription, topic=expected_topic,
         )
+        assert subscriber._gc_project_id == "rele-test"
 
     @patch.object(SubscriberClient, "create_subscription")
     def test_creates_subscription_with_custom_ack_deadline_when_provided(
diff --git a/tests/test_config.py b/tests/test_config.py
index 000aef0..b862449 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -58,7 +58,6 @@ class TestConfig:
         settings = {
             "APP_NAME": "rele",
             "SUB_PREFIX": "rele",
-            "GC_PROJECT_ID": project_id,
             "GC_CREDENTIALS_PATH": "tests/dummy-pub-sub-credentials.json",
             "MIDDLEWARE": ["rele.contrib.DjangoDBMiddleware"],
             "ENCODER": custom_encoder,
@@ -84,6 +83,28 @@ class TestConfig:
         assert isinstance(config.credentials, google.oauth2.service_account.Credentials)
         assert config.credentials.project_id == "rele-test"
 
+    def test_warns_when_gc_project_id_given(self, project_id):
+        settings = {
+            "GC_PROJECT_ID": project_id,
+            "GC_CREDENTIALS_PATH": "tests/dummy-pub-sub-credentials.json",
+        }
+
+        config = Config(settings)
+
+        with pytest.warns(DeprecationWarning):
+            assert config.gc_project_id == project_id
+
+    def test_uses_project_id_from_creds_when_no_project_id_given(self):
+        settings = {
+            "GC_CREDENTIALS_PATH": "tests/dummy-pub-sub-credentials.json",
+        }
+
+        config = Config(settings)
+
+        assert isinstance(config.credentials, google.oauth2.service_account.Credentials)
+        assert config.credentials.project_id == "rele-test"
+        assert config.gc_project_id == "rele-test"
+
     @patch.dict(os.environ, {"GOOGLE_APPLICATION_CREDENTIALS": ""})
     def test_sets_defaults(self):
         settings = {}
diff --git a/tests/test_worker.py b/tests/test_worker.py
index e6418b4..08b8ae7 100644
--- a/tests/test_worker.py
+++ b/tests/test_worker.py
@@ -1,4 +1,3 @@
-import os
 from concurrent import futures
 from unittest.mock import ANY, patch
 
@@ -113,23 +112,6 @@ class TestWorker:
         worker.setup()
 
         assert worker._subscriber._ack_deadline == custom_ack_deadline
-
-    @patch.dict(
-        os.environ,
-        {
-            "GOOGLE_APPLICATION_CREDENTIALS": os.path.dirname(
-                os.path.realpath(__file__)
-            )
-            + "/dummy-pub-sub-credentials.json"
-        },
-    )
-    @pytest.mark.usefixtures("mock_create_subscription")
-    def test_creates_without_config(self):
-        subscriptions = (sub_stub,)
-        worker = Worker(subscriptions)
-        worker.setup()
-
-        assert worker._subscriber._ack_deadline == 60
         assert worker._subscriber._gc_project_id == "rele-test"