Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ggrushka/DAR-385/tableau url limitation #131

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

grgilad
Copy link
Contributor

@grgilad grgilad commented Jul 4, 2024

Tableau API requests are limited to the following format -
url = f"{self._server.baseurl}/sites/{self._server.site_id}/{path}"

I'm introducing a new URL call which does not have sites in it's path. Since changing the code in it's entirety is a no go (both data-collector and apollo-agent), this allows to send the full path from the dc code.

@grgilad grgilad requested a review from a team as a code owner July 4, 2024 12:20
Copy link

linear bot commented Jul 4, 2024

@grgilad grgilad requested a review from mrostan July 4, 2024 12:22
@grgilad
Copy link
Contributor Author

grgilad commented Jul 4, 2024

@mrostan can you take a look, see if this hack makes sense?

Copy link
Contributor

@mrostan mrostan left a comment

Choose a reason for hiding this comment

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

@grgilad I think this is fine, but just wondering, what if we check for the path starting with "/" and use {baseurl}{path} in that case? that way we wouldn't be sending a whole url and potentially connecting to any other site
From the DC code it seems all paths we send are not starting with "/"

Copy link
Contributor

@mrostan mrostan left a comment

Choose a reason for hiding this comment

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

lgtm!

@grgilad
Copy link
Contributor Author

grgilad commented Jul 4, 2024

lgtm!

Thanks!

@grgilad grgilad merged commit 021f537 into main Jul 4, 2024
4 checks passed
@grgilad grgilad deleted the ggrushka/DAR-385/tableau_url_limitation branch July 4, 2024 13:05
@mrostan
Copy link
Contributor

mrostan commented Jul 4, 2024

@grgilad just in case you missed it, this is the release process for the agent: https://github.com/monte-carlo-data/apollo-agent?tab=readme-ov-file#release-process
You can use the previous release notes as a reference, we basically use the "generate release notes option" and make a few minor changes

@grgilad
Copy link
Contributor Author

grgilad commented Jul 4, 2024

@grgilad just in case you missed it, this is the release process for the agent: https://github.com/monte-carlo-data/apollo-agent?tab=readme-ov-file#release-process You can use the previous release notes as a reference, we basically use the "generate release notes option" and make a few minor changes

Thanks, will do, want to wait a bit and see if more changes are required.

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