[TDL-15656] Added Support for dev mode by vishalp-dev · Pull Request #41 · singer-io/tap-eloqua

vishalp-dev · 2022-09-06T03:23:05Z

Description of change

Add support for -dev / --dev cli param,

prevent refresh_token expiry while running in dev-mode
stores the current access_token and expiry in config
exits tap if existing access_token is not available
Added util to write back to config file
Added Unit tests for dev-mode
Updated Singer-Python Dependancy to 5.13 (dev mode support)
Updated CirlceCi Config yaml

Manual QA steps

First run the tap normally using the standard params
Check the config file for added keys access_token & expires_in
Run the tap using the --dev or '-dev' param
Check the loglife for dev mode related logs

Risks (Low)

P.R Dependant on #158

Tap will exit for existing connections when run in dev mode if access_token is not available

Rollback steps

revert this branch

tap_eloqua/utils.py

tap_eloqua/__init__.py

tap_eloqua/client.py

tap_eloqua/sync.py

tap_eloqua/client.py

Co-authored-by: Rushikesh Todkar <98420315+RushiT0122@users.noreply.github.com>

tap_eloqua/client.py

rdeshmukh15 · 2023-01-23T10:12:33Z

tests/unittests/test_devmode.py

+        with open(self.tmp_config_path, "w") as ff:
+            json.dump(self.base_config, ff)


I will strongly suggest deleting the temp config once the unit test is executed. If we don't delete it, I think it will keep on accumulating the temp configs (as the everyday job runs) within circleci server.

agreed, but as the cci runner is temporary provisioned and the files created during testing are not preserved.
it will be autodeleted

RushiT0122 · 2023-01-23T10:06:54Z

tap_eloqua/client.py

-                          Server5xxError,
-                          max_tries=5,
-                          factor=2)
+    @backoff.on_exception(backoff.expo,Server5xxError,5,factor=2)


Suggested change

@backoff.on_exception(backoff.expo,Server5xxError,5,factor=2)

@backoff.on_exception(backoff.expo,

Server5xxError,

max_tries=5,

factor=2)

RushiT0122 · 2023-01-23T10:12:38Z

tap_eloqua/client.py

+            except KeyError as err:
+                raise Exception("Unable to locate key in config") from err
+            if not self.__access_token or self.__expires < now():
+                raise Exception("Access Token in config is expired, unable to authenticate in dev mode")


Use either ex or e as a convention for exception.

Suggested change

except KeyError as err:

raise Exception("Unable to locate key in config") from err

if not self.__access_token or self.__expires < now():

raise Exception("Access Token in config is expired, unable to authenticate in dev mode")

except KeyError as ex:

raise Exception("Unable to locate key in config") from ex

if not self.__access_token or self.__expires < now():

raise Exception("Access Token in config is expired, unable to authenticate in dev mode")

RushiT0122 · 2023-01-23T10:13:43Z

tap_eloqua/client.py


    def post(self, path, **kwargs):
        return self.request('POST', path=path, **kwargs)
+


Remove this extra line.

RushiT0122 · 2023-01-23T10:19:10Z

tap_eloqua/client.py

-        expires_seconds = data['expires_in'] - 10 # pad by 10 seconds
-        self.__expires = datetime.utcnow() + timedelta(seconds=expires_seconds)
+        if not self.dev_mode:
+            update_config_keys = {"refresh_token":self.__refresh_token,"access_token":self.__access_token,"expires_in": strftime(self.__expires)}


Reduce the statement length.

Suggested change

update_config_keys = {"refresh_token":self.__refresh_token,"access_token":self.__access_token,"expires_in": strftime(self.__expires)}

update_config_keys = {"refresh_token": self.__refresh_token,

"access_token": self.__access_token,

"expires_in": strftime(self.__expires)}

RushiT0122 · 2023-01-23T10:43:04Z

tap_eloqua/sync.py

    finished = False
    sync_start = pendulum.now('UTC')
-    end_date = sync_start
+    end_date, last_sync_date = sync_start, None


Are these changes in sync.py dev-mode related? If not can we keep it separate from dev-mode changes.

And if these are required changes then update the description on this PR.

@RushiT0122
These are not dev mode related changes, but if you see the code from Line:380 - Line:403
last_sync_date was declared on Line:385 in the try statement, but accessed outside its declared scope in the except block, hence it was introduced here.

I'm pretty sure this is a problem in other languages, but not python

>>> for i in range(3): ... try: ... print("About to initialize `my_variable`") ... my_variable = i ... print(f"my_variable: {my_variable}") ... raise RuntimeError(f"Problem occured with {my_variable}") ... except Exception as err: ... print(f"Caught {type(err)} with message {str(err)}") ... print(f"Current value of my_variable {my_variable}") ... About to initialize `my_variable` my_variable: 0 Caught <class 'RuntimeError'> with message Problem occured with 0 Current value of my_variable 0 About to initialize `my_variable` my_variable: 1 Caught <class 'RuntimeError'> with message Problem occured with 1 Current value of my_variable 1 About to initialize `my_variable` my_variable: 2 Caught <class 'RuntimeError'> with message Problem occured with 2 Current value of my_variable 2

RushiT0122 · 2023-01-23T10:47:50Z

tap_eloqua/utils.py

+LOGGER = get_logger()
+
+
+def write_config(config_path,data :Dict) -> Dict:


Please check this comment on tap-deputy: singer-io/tap-deputy#3 (comment)

made required changes

RushiT0122 · 2023-01-23T11:20:07Z

tests/unittests/test_devmode.py

+        try:
+            client = EloquaClient(config_path=self.tmp_config_path,dev_mode=True,config=self.base_config)
+
+            client.get_access_token()
+        except Exception as error:
+            self.assertEqual(str(error), "Unable to locate key in config")


Use with self.assertRaises() unittest construct here.

I can't use assertRaises here, because we can only check the "type" of the exception with it.
But in this case, all exceptions are raised with "Exception" class and I want to validate the message-string that is raised with it.

RushiT0122 · 2023-01-23T11:21:10Z

tests/unittests/test_devmode.py

+            self.assertEqual(str(error), "Unable to locate key in config")
+
+    @patch("requests.Session.post", side_effect=mocked_auth_post)
+    def test_client_valid_token(self, mocked_auth_post):


Suggested change

def test_client_valid_token(self, mocked_auth_post):

def test_client_valid_access_token(self, mocked_auth_post):

RushiT0122 · 2023-01-23T11:21:34Z

tests/unittests/test_devmode.py

+        client.get_access_token()
+
+    @patch("requests.Session.post", side_effect=mocked_auth_post)
+    def test_client_invalid_token(self, mocked_auth_post):


Suggested change

def test_client_invalid_token(self, mocked_auth_post):

def test_client_invalid_access_token(self, mocked_auth_post):

RushiT0122 · 2023-01-23T11:22:13Z

tests/unittests/test_devmode.py

+        try:
+            client = EloquaClient(config_path=self.tmp_config_path,dev_mode=True,config=config_sample)
+            client.get_access_token()
+        except Exception as error:
+            self.assertEqual(str(error), "Access Token in config is expired, unable to authenticate in dev mode")


Use with self.assertRaises() unittest construct here.

I can't use assertRaises here, because we can only check the "type" of the exception with it.
But in this case, all exceptions are raised with "Exception" class and I want to validate the message-string that is raised with it.

Docs for assertRaises. These docs show how you can use a context manager with assertRaises. And they show that the exception object is a property on the manager, which means we can do exactly what you want to do

Here's how you use it

$ python3 Python 3.10.9 (main, Dec 15 2022, 18:25:35) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin Type "help", "copyright", "credits" or "license" for more information. # start setup >>> import unittest >>> class some_test(unittest.TestCase): ... pass ... >>> my_test = some_test() # Prove assertRaises works >>> with my_test.assertRaises(Exception): ... 1+1 ... 2 Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/local/Cellar/python@3.10/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/unittest/case.py", line 226, in __exit__ self._raiseFailure("{} not raised".format(exc_name)) File "/usr/local/Cellar/python@3.10/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/unittest/case.py", line 163, in _raiseFailure raise self.test_case.failureException(msg) AssertionError: Exception not raised # Prove the test can pass >>> with my_test.assertRaises(Exception) as context_manager: ... raise Exception("this is a test") ... # Show we have the exception >>> actual_exception = context_manager.exception >>> actual_exception Exception('this is a test') >>> str(actual_exception) 'this is a test'

luandy64 · 2023-01-05T21:15:44Z

tests/unittests/test_devmode.py

+            self.assertEqual(True, "access_token" in config)
+            self.assertEqual(True, "refresh_token" in config)
+
+    def test_devmode_accesstoken_absent(self, *args, **kwargs):


It looks like *args and **kwargs is unused

luandy64

Overall, there are a lot of little changes to make. Functionally, the code seems fine and a lot like other dev-mode implementations.

But because code is written once and read over and over, I think it's very important that we think about cleaning up the style once the logic is complete. It's nice to reference any of the style guides out on the internet (they all basically say the same thing).

luandy64 · 2023-01-25T14:17:40Z

.circleci/config.yml

+          name: 'pylint'
+          command: |
+            source /usr/local/share/virtualenvs/tap-eloqua/bin/activate
+            pylint tap_eloqua -d C,R,W


Should we use the standard list of disables?

luandy64 · 2023-01-25T14:28:36Z

tap_eloqua/sync.py

    finished = False
    sync_start = pendulum.now('UTC')
-    end_date = sync_start
+    end_date, last_sync_date = sync_start, None


I'm pretty sure this is a problem in other languages, but not python

>>> for i in range(3): ... try: ... print("About to initialize `my_variable`") ... my_variable = i ... print(f"my_variable: {my_variable}") ... raise RuntimeError(f"Problem occured with {my_variable}") ... except Exception as err: ... print(f"Caught {type(err)} with message {str(err)}") ... print(f"Current value of my_variable {my_variable}") ... About to initialize `my_variable` my_variable: 0 Caught <class 'RuntimeError'> with message Problem occured with 0 Current value of my_variable 0 About to initialize `my_variable` my_variable: 1 Caught <class 'RuntimeError'> with message Problem occured with 1 Current value of my_variable 1 About to initialize `my_variable` my_variable: 2 Caught <class 'RuntimeError'> with message Problem occured with 2 Current value of my_variable 2

luandy64 · 2023-01-25T14:29:30Z

tap_eloqua/utils.py

@@ -0,0 +1,18 @@
+from typing import Dict


This is unused

luandy64 · 2023-01-25T14:31:52Z

tests/unittests/test_devmode.py

+    @patch("requests.Session.post", side_effect=mocked_auth_post)
+    def test_client_without_dev_mode(self, mocked_auth_post):
+        """checks if the client can write refresh token and expiry to
+        config."""


Why is this line wrapped if line 23 isn't?

luandy64 · 2023-01-25T14:36:44Z

tests/unittests/test_devmode.py

+            self.assertEqual(True, "expires_in" in config)
+            self.assertEqual(True, "access_token" in config)
+            self.assertEqual(True, "refresh_token" in config)


I think there are 2 better options here:

"expires_in" in config is a boolean. So these are equivalent

self.assertEqual(True, "expires_in" in config) self.assert("expires_in" in config)

https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn

luandy64 · 2023-01-25T14:45:32Z

tap_eloqua/client.py

+        if self.dev_mode:
+            try:
+                self.__access_token = self.config['access_token']
+                self.__expires=strptime_to_utc(self.config['expires_in'])


Suggested change

self.__expires=strptime_to_utc(self.config['expires_in'])

self.__expires = strptime_to_utc(self.config['expires_in'])

luandy64 · 2023-01-25T14:45:59Z

tap_eloqua/client.py

+                                    "refresh_token":self.__refresh_token,
+                                    "access_token":self.__access_token,
+                                    "expires_in": strftime(self.__expires)


Suggested change

"refresh_token":self.__refresh_token,

"access_token":self.__access_token,

"expires_in": strftime(self.__expires)

"refresh_token": self.__refresh_token,

"access_token": self.__access_token,

"expires_in": strftime(self.__expires)

luandy64 · 2023-01-25T14:46:12Z

tap_eloqua/client.py

+                                    "access_token":self.__access_token,
+                                    "expires_in": strftime(self.__expires)
+                                }
+            self.config = write_config(self.__config_path,update_config_keys)


Suggested change

self.config = write_config(self.__config_path,update_config_keys)

self.config = write_config(self.__config_path, update_config_keys)

luandy64 · 2023-01-25T14:46:22Z

tap_eloqua/client.py

-        data = self.request('GET',
-                            url='https://login.eloqua.com/id',
-                            endpoint='base_url')
+        data = self.request('GET',url='https://login.eloqua.com/id',endpoint='base_url')


Suggested change

data = self.request('GET',url='https://login.eloqua.com/id',endpoint='base_url')

data = self.request('GET', url='https://login.eloqua.com/id', endpoint='base_url')

luandy64 · 2023-01-25T14:47:03Z

tests/unittests/test_devmode.py

+            json.dump(self.base_config, ff)
+
+
+        client = EloquaClient(config_path=self.tmp_config_path,dev_mode=False,config=self.base_config)


Suggested change

client = EloquaClient(config_path=self.tmp_config_path,dev_mode=False,config=self.base_config)

client = EloquaClient(config_path=self.tmp_config_path, dev_mode=False, config=self.base_config)

vishalp-dev added 3 commits September 5, 2022 20:56

added support for dev-mode

cccfc1b

added expiry time to config

d586f25

added unittest for devmode

2581588

vishalp-dev marked this pull request as ready for review September 13, 2022 03:10

vishalp-dev requested review from RushiT0122, bryantgray, dbshah1212 and dmosorast and removed request for dmosorast September 13, 2022 03:11

dbshah1212 reviewed Sep 13, 2022

View reviewed changes

tap_eloqua/utils.py Outdated Show resolved Hide resolved

added missing function docs

4ad0dac

vishalp-dev requested a review from dmosorast September 14, 2022 17:11

vishalp-dev added 5 commits September 14, 2022 22:48

added unit tests to circleCI

1481ba4

added dev dependencies

e6ee98b

update tap-tester image

9099b0b

fixed pylint issue

5b0690b

fixed pylint issues

eddf3ba

RushiT0122 reviewed Sep 16, 2022

View reviewed changes

tap_eloqua/__init__.py Outdated Show resolved Hide resolved

RushiT0122 requested changes Sep 16, 2022

View reviewed changes

tap_eloqua/client.py Outdated Show resolved Hide resolved

tap_eloqua/client.py Outdated Show resolved Hide resolved

tap_eloqua/client.py Outdated Show resolved Hide resolved

tap_eloqua/sync.py Outdated Show resolved Hide resolved

tap_eloqua/client.py Show resolved Hide resolved

fix review comments

2ae8cb3

Co-authored-by: Rushikesh Todkar <98420315+RushiT0122@users.noreply.github.com>

dmosorast reviewed Oct 10, 2022

View reviewed changes

tap_eloqua/client.py Outdated Show resolved Hide resolved

vishalp-dev added 9 commits October 27, 2022 23:53

updated dev mode implementation

39c9754

updated formatting

59c5b74

fixed review issues

330d464

updated singer_python version & fixed unit tests

264e0d0

fixed unittests

cb905b0

removed readconfig

31de7b0

removed unused import

e02e8a1

fixed linting issues

d75aac7

fixed cci

53ff291

fixed unit_test issue

5269472

vishalp-dev requested review from rdeshmukh15 and sgandhi1311 January 23, 2023 09:36

sgandhi1311 approved these changes Jan 23, 2023

View reviewed changes

RushiT0122 self-requested a review January 23, 2023 09:53

rdeshmukh15 reviewed Jan 23, 2023

View reviewed changes

RushiT0122 requested changes Jan 23, 2023

View reviewed changes

RushiT0122 reviewed Jan 23, 2023

View reviewed changes

vishalp-dev added 4 commits January 23, 2023 18:29

fixed PR review comments

475dff9

fixed pylint issue

6021e28

fix issue with unit test

13bcef3

added setup function for UT

e1cbe5b

luandy64 reviewed Jan 25, 2023

View reviewed changes

		with open(self.tmp_config_path, "w") as ff:
		json.dump(self.base_config, ff)


		def post(self, path, **kwargs):
		return self.request('POST', path=path, **kwargs)

		LOGGER = get_logger()


		def write_config(config_path,data :Dict) -> Dict:

	def test_client_valid_token(self, mocked_auth_post):
	def test_client_valid_access_token(self, mocked_auth_post):

	def test_client_invalid_token(self, mocked_auth_post):
	def test_client_invalid_access_token(self, mocked_auth_post):

	self.__expires=strptime_to_utc(self.config['expires_in'])
	self.__expires = strptime_to_utc(self.config['expires_in'])

	self.config = write_config(self.__config_path,update_config_keys)
	self.config = write_config(self.__config_path, update_config_keys)

	data = self.request('GET',url='https://login.eloqua.com/id',endpoint='base_url')
	data = self.request('GET', url='https://login.eloqua.com/id', endpoint='base_url')

		json.dump(self.base_config, ff)


		client = EloquaClient(config_path=self.tmp_config_path,dev_mode=False,config=self.base_config)

Conversation

vishalp-dev commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

Manual QA steps

Risks (Low)

P.R Dependant on #158

Rollback steps

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RushiT0122 Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RushiT0122 Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luandy64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishalp-dev commented Sep 6, 2022 •

edited

Loading

RushiT0122 Jan 23, 2023 •

edited

Loading

RushiT0122 Jan 23, 2023 •

edited

Loading