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.

Sign up for GitHub

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

Jump to bottom

fix(connections) Harden adding /gms to connections in backend #10942

Merged
merged 1 commit into from
Jul 19, 2024
Merged

fix(connections) Harden adding /gms to connections in backend #10942

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions metadata-ingestion/src/datahub/cli/cli_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,9 @@ def _ensure_valid_gms_url_acryl_cloud(url: str) -> str:
url = url.replace("http://", "https://")
if url.endswith("acryl.io"):
url = f"{url}/gms"
elif url.endswith("acryl.io/"):
url = f"{url}gms"

Copy link
Collaborator

Choose a reason for hiding this comment

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

we actually don't need this - fixup_gms_url is what calls _ensure_valid_gms_url_acryl_cloud, and already removes the trailing slash

that said, it probably would be good to add some tests around this so we don't have a regression

Copy link
Collaborator

Choose a reason for hiding this comment

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

@chriscollins3456 test_cli_utils.py has some unit tests that you should probably add to if you found cases where it wasn't doing what it was supposed to do.

return url


Expand Down
Loading