Refactoring language_container_deployer #162

ahsimb · 2023-12-01T14:04:50Z

closes #159

All Submissions:

Is the title of the Pull Request correct?
Is the title of the corresponding issue correct?
Have you updated the changelog?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Are you mentioning the issue which this PullRequest fixes ("Fixes...")
Before you merge don't forget to run tests in AWS CodeBuild, by adding [CodeBuild] to the commit message

tkilias · 2023-12-01T14:31:06Z

tests/unit_tests/deployment/test_language_container_deployer.py

+
+    deployer.upload_container = MagicMock()
+    deployer.activate_container = MagicMock()
+    deployer.get_language_settings = MagicMock()


Instead of making the function get_language_settings public and overwriting it with a mock, I would suggest to extract completely from the deployer class and injecting it into the constructor as a parameter with default to its actual implementation

Maybe, actually extract all three functions (Upload, activate, get_language_settings) in one extra class.

For the moment, we could keep it this way, but long-term these functions might be of interest independent of the deployer. We can then extract them.

tkilias · 2023-12-01T15:27:55Z

exasol_transformers_extension/deployment/language_container_deployer_cli.py

@@ -52,6 +26,8 @@ def run_deployer(deployer, upload_container: bool = True,
                  utils.DB_PASSWORD_ENVIRONMENT_VARIABLE, ""))


The click Options are not yet packed into @add_options(create_deployer_option(utils.DB_PASSWORD_ENVIRONMENT_VARIABLE, utils.BUCKETFS_PASSWORD_ENVIRONMENT_VARIABLE), but I think we can only do it during reintegration. Maybe in the meantime we should copy the add_options from itde to here. This prepare this already.

exasol_transformers_extension/deployment/language_container_deployer.py

tkilias · 2023-12-06T10:53:53Z

exasol_transformers_extension/deployment/te_language_container_deployer.py

+    SLC_NAME = "exasol_transformers_extension_container_release.tar.gz"
+    GH_RELEASE_URL = "https://github.com/exasol/transformers-extension/releases/download"
+
+    def download_from_git_and_run(self, version: str,


Thinking about, we could move this as well to the LanguageContainerDeployer and use attributes for the url and slc name which we set in the constructor

tkilias · 2023-12-06T10:54:04Z

exasol_transformers_extension/deployment/te_language_container_deployer.py

+    SLC_NAME = "exasol_transformers_extension_container_release.tar.gz"
+    GH_RELEASE_URL = "https://github.com/exasol/transformers-extension/releases/download"
+
+    def download_from_git_and_run(self, version: str,


Suggested change

def download_from_git_and_run(self, version: str,

def download_from_github_and_run(self, version: str,

tkilias · 2023-12-06T13:07:06Z

tests/unit_tests/deployment/test_language_container_deployer.py

+
+    deployer.upload_container = MagicMock()
+    deployer.activate_container = MagicMock()
+    deployer.get_language_settings = MagicMock()


For the moment, we could keep it this way, but long-term these functions might be of interest independent of the deployer. We can then extract them.

…ployer.py Co-authored-by: Torsten Kilias <[email protected]>

ahsimb · 2023-12-07T14:40:52Z

This commit is NOT ready to be merged. Pushing it just to discuss some ideas.

tkilias · 2023-12-08T12:30:13Z

exasol_transformers_extension/deployment/language_container_deployer_cli.py


        def update_parameter(parameter_name: str, formatter: str) -> None:
            param_formatter = ctx.params.get(parameter_name, formatter)
            if param_formatter:
+                # Enclose in double curly brackets all other parameters in the formatting string,
+                # to avoid the missing parameters' error.
+                pattern = r'\{(?!' + param.name + r'\})\w+\}'


an example how the pattern looks at the end and what it matches might be useful

tkilias · 2023-12-08T14:27:12Z

tests/unit_tests/deployment/test_language_container_deployer_cli.py

+    _ParameterFormatters, CustomizableParameters)
+
+
+def test_parameter_formatters_1param():


wow, I like that

tkilias · 2023-12-11T09:25:07Z

exasol_transformers_extension/deployment/language_container_deployer_cli.py

-                # to avoid the missing parameters' error.
+                # to avoid the missing parameters' error. Below is an example of a formatter string
+                # before and after applying the regex, assuming the current parameter is 'version'.
+                # 'something-with-{version}/tailored-for-{user}' => 'something-with-{version}/tailored-for-{{user}}'


did you actually change the wrong parameter

The pattern in this example would be \{(?!version\})\w+\} and it matches all substrings {...} that are not {version} .

ahsimb · 2023-12-11T11:21:33Z

This is just an explanation of a generic usage. We currently don't have a use case with more than 1 parameter.

Refactoring language_container_deployer

88c4647

ahsimb requested a review from tkilias December 1, 2023 14:06

ahsimb self-assigned this Dec 1, 2023

ahsimb added the refactoring label Dec 1, 2023

tkilias requested changes Dec 1, 2023

View reviewed changes

tkilias requested changes Dec 6, 2023

View reviewed changes

ahsimb and others added 4 commits December 7, 2023 08:04

Update exasol_transformers_extension/deployment/language_container_de…

8721eaa

…ployer.py Co-authored-by: Torsten Kilias <[email protected]>

Update exasol_transformers_extension/deployment/language_container_de…

5946073

…ployer.py Co-authored-by: Torsten Kilias <[email protected]>

Update exasol_transformers_extension/deployment/language_container_de…

98b2591

…ployer.py Co-authored-by: Torsten Kilias <[email protected]>

Generalising the cli

7d4f31b

Generalising the cli finalized

7e4b099

tkilias requested changes Dec 8, 2023

View reviewed changes

Little change in a doc and [CodeBuild]

05236b0

tkilias reviewed Dec 11, 2023

View reviewed changes

Another little change in a doc and [CodeBuild]

4fbb826

tkilias approved these changes Dec 11, 2023

View reviewed changes

tkilias and others added 2 commits December 11, 2023 14:10

Merge branch 'main' into refactoring/159-prepare-deployment-migration

914f862

Updating the branch [CodeBuild]

16581a8

ahsimb merged commit e736b42 into main Dec 11, 2023
3 checks passed

ahsimb deleted the refactoring/159-prepare-deployment-migration branch December 11, 2023 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring language_container_deployer #162

Refactoring language_container_deployer #162

ahsimb commented Dec 1, 2023 •

edited

Loading

tkilias Dec 1, 2023

tkilias Dec 1, 2023

tkilias Dec 6, 2023

tkilias Dec 1, 2023 •

edited

Loading

tkilias Dec 6, 2023

tkilias Dec 6, 2023

tkilias Dec 6, 2023

ahsimb commented Dec 7, 2023

tkilias Dec 8, 2023

tkilias Dec 8, 2023

tkilias Dec 11, 2023

tkilias Dec 11, 2023

ahsimb commented Dec 11, 2023

		@@ -52,6 +26,8 @@ def run_deployer(deployer, upload_container: bool = True,
		utils.DB_PASSWORD_ENVIRONMENT_VARIABLE, ""))

	def download_from_git_and_run(self, version: str,
	def download_from_github_and_run(self, version: str,

		_ParameterFormatters, CustomizableParameters)


		def test_parameter_formatters_1param():

Refactoring language_container_deployer #162

Refactoring language_container_deployer #162

Conversation

ahsimb commented Dec 1, 2023 • edited Loading

All Submissions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkilias Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsimb commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsimb commented Dec 11, 2023

ahsimb commented Dec 1, 2023 •

edited

Loading

tkilias Dec 1, 2023 •

edited

Loading