Skip to content

[WIP] Looker integration#87

Merged
pawelpinkos merged 16 commits into
developfrom
looker-integration
Oct 6, 2022
Merged

[WIP] Looker integration#87
pawelpinkos merged 16 commits into
developfrom
looker-integration

Conversation

@pawelpinkos
Copy link
Copy Markdown
Collaborator

Looker integration

Resolves <issue nr here>


Keep in mind:

Comment thread data_pipelines_cli/cli_commands/bi.py Outdated
"--docker-args", type=str, required=False, help="Args required to build project in json format"
)
@click.option(
"--bi-build",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about turning it on and off from bi.yml instead. Maybe both could be used. We would like to control it without changes in CICD process :)

help="Whether to push to BI",
)
@click.option(
"--key-path",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's name it --bi-git-key-path maybe

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread data_pipelines_cli/cli_commands/deploy.py Outdated
Comment thread setup.py Outdated
"fsspec",
"packaging>=20.4,<21.0",
"colorama==0.4.4",
"dbt2looker==0.10.0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put it in "looker" target. Not everyone wants to install it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, done

Comment thread tests/cli_commands/test_deploy.py Outdated
):
with self.assertRaises(DependencyNotInstalledError):
DeployCommand("base", False, self.storage_uri, self.provider_args, True).deploy()
DeployCommand("base", False, self.storage_uri, self.provider_args, True, False, None).deploy()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are 2 parameters needed I would introduce separate objects for example bi_parameters and sent it instead the one you added (open-close principle).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bi configs moved to bi.yml, no longer needed more than one param for bi

@pawelpinkos pawelpinkos merged commit 26acc59 into develop Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants