Refactoring/7 use new bfs interface #9
Conversation
# Conflicts: # exasol/python_extension_common/deployment/language_container_deployer.py # exasol/python_extension_common/deployment/language_container_deployer_cli.py # pyproject.toml # test/unit/deployment/test_language_container_deployer.py # test/unit/deployment/test_language_container_deployer_cli.py
| bucketfs_host: Optional[str] = None, bucketfs_port: Optional[int] = None, | ||
| bucketfs_name: Optional[str] = None, bucket: Optional[str] = None, | ||
| bucketfs_user: Optional[str] = None, bucketfs_password: Optional[str] = None, | ||
| bucketfs_use_https: bool = True, | ||
| saas_url: Optional[str] = None, | ||
| saas_account_id: Optional[str] = None, saas_database_id: Optional[str] = None, | ||
| saas_token: Optional[str] = None, | ||
| path_in_bucket: str = '', |
There was a problem hiding this comment.
Just an idea: Would it be better to have two different class methods: createOnPrem() and createSaas()?
There was a problem hiding this comment.
Not sure. The decision of which way to go needs to be taken somewhere. By making two different methods we are pushing this decision to the caller. From the caller's perspective, it might be easier to grab all available parameters, e.g. defined in the config, pass them over to the single point of call and let it make sense of these parameters.
There was a problem hiding this comment.
I agree that we would move the responsibility to the client. I'am not sure about the client's case. However, if the client knows that the connection is on-prem or SaaS, it would be cumbersome to pass all the unnecessary parameters.
There was a problem hiding this comment.
You don't have to pass the unnecessary parameters. They are optional. For example, if you know that you work with SaaS the call might look like the following:
deployer = LanguageContainerDeployer.create(dsn, db_user, db_password, language_alias, saas_url='...', saas_acount_id='...', saas_database_id='...', saas_token='...')
There was a problem hiding this comment.
Well :), except that the dsn, db_user and db_password are not really required for SaaS. They are derivatives of the other SaaS parameters. But we will sort it out in subsequent PRs. Need to first merge the saas-api-python with the helper function that works out the saas connection parameters.
There was a problem hiding this comment.
Or maybe I should merge it first and then do things property in this PR,
There was a problem hiding this comment.
Same here, maybe have two different commands, one for OnPrem and one for SaaS
There was a problem hiding this comment.
Again, I think from the caller's (a human in this case) point of view it's better to have one cli command and different parameters, rather than two. One less name to remember, the rest is the same.
| for extension in ARCHIVE_EXTENSIONS: | ||
| if bucket_file.endswith(extension): | ||
| bucket_file = bucket_file[: -len(extension)] | ||
| break | ||
|
|
||
| file_path = bucket_base_path / bucket_file | ||
| return PurePosixPath(file_path.as_udf_path()) |
There was a problem hiding this comment.
hm, this might be actually logic that needs to go into PathLike.as_udf_path()
There was a problem hiding this comment.
Not sure about that. The PathLike functions return either a string or another PathLike. The PathLike != PurePosixPath.
There was a problem hiding this comment.
ah, I didn't mean the PurePosixPath part, I meant the extension part for archives
There was a problem hiding this comment.
I am a bit confused about what happens with an archive. It gets unzipped into the .dist directory. What do we actually see in the udf, the zipped or unzipped file? If it's the latter, then yes we should move that code to the backetfs. Does the dist get hidden in this case, somehow? I think it's better to do this fix in a separate PR because it requires a change in another repo.
exasol/python_extension_common/deployment/language_container_deployer.py
Outdated
Show resolved
Hide resolved
exasol/python_extension_common/deployment/language_container_deployer_cli.py
Show resolved
Hide resolved
| default=lambda: os.environ.get(SAAS_ACCOUNT_ID_ENVIRONMENT_VARIABLE)) | ||
| @click.option('--saas-database-id', type=str, | ||
| default=lambda: os.environ.get(SAAS_DATABASE_ID_ENVIRONMENT_VARIABLE)) | ||
| @click.option('--saas-token', type=str, |
There was a problem hiding this comment.
for the token and the passwords, we should maybe add an input prompt in another PR, when they are not provided as cli argument or environment variable
There was a problem hiding this comment.
These values are quite long, entering them manually would be torture.
There was a problem hiding this comment.
you can copy them into the input prompt. The input prompt is usually used, when you don't want to leak your credentials in your shell history. However, as I said, this can be done later. I think, it simply enough to create a ticket for it.
There was a problem hiding this comment.
I am not sure I understood. According to the click doc, you can offer the user a choice, either to accept the default value stored in a config or env. variable, or enter it manually from a terminal. My point is that entering the saas parameters manually is not practical because they are too long. Hence we don't need a prompt here. It may be useful for the db password though.
closes #7