Feature/initial pr #8
Conversation
export analysis, import template, publish dashboard from template Add input and output directories.
core/cli.py
Outdated
|
|
||
|
|
||
| @click.command() | ||
| @click.option("--aws-profile", required=True, help="The AWS account profile") |
There was a problem hiding this comment.
Why not rely on the built-in boto3 methods of picking a profile and credentials?
https://boto3.amazonaws.com/v1/documentation/api/latest/guide/configuration.html#using-environment-variables
There was a problem hiding this comment.
I suppose because I wanted to require that the user to make an explicit choice - ie I don't want default aws configs to be used by mistake.
There was a problem hiding this comment.
But in looking more closely at this, there really is no good reason not to use AWS_PROFILE or other standard approaches. Plus it fits better with our terraform usage. I will fix.
core/cli.py
Outdated
| """ | ||
| Exports a template and dependent data sets based on the specified analysis to JSON files. | ||
| """ | ||
| click.echo(f"Create version") |
There was a problem hiding this comment.
Can we change from echo to logging. We may run this locally for now, but we may have this as a CI trigger in the future in which case we may want finer control over the output.
There was a problem hiding this comment.
Yes. Good idea.
core/operation/baseoperation.py
Outdated
| try: | ||
| response = self._qs_client.create_template(**template_data) | ||
| except self._qs_client.exceptions.ResourceExistsException as e: | ||
| response = self._qs_client.update_template(**template_data) |
There was a problem hiding this comment.
The API definition for updateTemplate doesn't seem to have "permissions" and "tags" as parameters that the createTemplate API has, should we looking to either
- raise an exception if they exist
- or delete the parameters before updating the template
There was a problem hiding this comment.
I think it makes more sense to delete the template (or dataset) if it exists and recreate it. That way we don't have to deal with tags and permissions updates separately.
core/operation/baseoperation.py
Outdated
| f"Unexpected response from trying to create/update template : {json.dumps(response, indent=4)} " | ||
| ) | ||
| else: | ||
| return response["Arn"], response["VersionArn"], response["TemplateId"] |
There was a problem hiding this comment.
This is more of a code style opinion, when returning multiple values it is easier to maintain the code when returning dataclasses of named and typed information as opposed to tuples or data
There was a problem hiding this comment.
Example.
@dataclass
class TemplateResponse:
arn: str
version_arn: str
template_id: str
...
def ....
return TemplateResponse(arn, version_arn, template_id)| retry(verify_success) | ||
|
|
||
| # get the newly created template definition | ||
| self._log.info(f"Writing template definition response to disk") |
There was a problem hiding this comment.
Can we add the filename to the log, so it is easy to find for the user.
| with open(template_file) as template_file: | ||
| template_data = json.loads(template_file.read()) | ||
|
|
||
| # create namespace if not exists |
| try: | ||
| response = self._qs_client.create_data_set(**dataset_definition) | ||
| except self._qs_client.exceptions.ResourceExistsException as e: | ||
| response = self._qs_client.update_data_set(**dataset_definition) |
There was a problem hiding this comment.
I did not check, but maybe the create and update APIs have differing parameters here as well?
There was a problem hiding this comment.
Same as above: I will fix by deleting and re-creating.
| from time import sleep | ||
|
|
||
|
|
||
| def retry(func) -> bool: |
There was a problem hiding this comment.
There are libraries that provide configurable methods of retrying till success, eg. https://github.com/invl/retry
If we don't need the configurability of these libraries we can stick to this implementation.
There was a problem hiding this comment.
For the time being I don't think we need configurability.
| :param val: | ||
| :return: | ||
| """ | ||
| if key in mydict: |
There was a problem hiding this comment.
Imaginary edge case: What if key is not a str but a list or dict, should we safeguard against replacing such values?
There was a problem hiding this comment.
It's case I don't think we need to worry about.
| class TestExportAnalysisOperation: | ||
| def test(self): | ||
| analysis_id = "my-quicksight-analysis-id" | ||
| output_dir = "/tmp/test-output" |
There was a problem hiding this comment.
A more platform independent API exists under https://docs.python.org/3/library/tempfile.html
Or you can use the mocking strategy https://docs.python.org/3/library/unittest.mock.html#mock-open
…t permissions and tags are overwritten properly.
There was a problem hiding this comment.
@RishiDiwanTT : Thanks for the great feedback. I've updated the PR with your suggested changes. I also added tests to cover import and publish.
|
@RishiDiwanTT : can you give the updates a quick look: if all is well, approve it and I'll merge it. |
|
|
||
| # there can be some latency between the completion of the deletion command | ||
| # and the complete backend deletion operation. | ||
| time.sleep(3) |
There was a problem hiding this comment.
Maybe we should add a log here so we can track when resources were deleted
Initial pull request for the palace-quicksight command-line tool.
It includes 3 functions:
The process has been tested manually in the tpp-dev quicksight account.
You can run those tests against the dev environment yourself by running the following commands:
Export Analysis
Import Template
Publish Template