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

Fix a small problem that prevented marathon full path from being properly built #2620

Merged
merged 1 commit into from Jul 26, 2016

Conversation

ensonik
Copy link
Contributor

@ensonik ensonik commented Jun 23, 2016

…properly built when the base url included a path.

As tested in the REPL, the fix will properly join base url's and paths:

  >>> urljoin("http://marathon.mesos", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos/service/marathon/", "v2/apps")
  'http://marathon.mesos/service/marathon/v2/apps'

  >>> urljoin("http://marathon.mesos", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos/", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos:8080", "v2/apps")
  'http://marathon.mesos:8080/v2/apps'

  >>> urljoin("http://marathon.mesos:8080/", "v2/apps")
  'http://marathon.mesos:8080/v2/apps'

Care needs to be taken to include a trailing slash in the base URL when there is a path included.

  >>> urljoin("http://marathon.mesos/service/marathon", "v2/apps")
  'http://marathon.mesos/service/v2/apps' # This is wrong. Include trailing slash to fix.

…properly built when the base url included a path.

As tested in the REPL, the fix will properly join base url's and paths:
  >>> urljoin("http://marathon.mesos", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos/service/marathon/", "v2/apps")
  'http://marathon.mesos/service/marathon/v2/apps'

  >>> urljoin("http://marathon.mesos", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos/", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos:8080", "v2/apps")
  'http://marathon.mesos:8080/v2/apps'

  >>> urljoin("http://marathon.mesos:8080/", "v2/apps")
  'http://marathon.mesos:8080/v2/apps'

Care needs to be taken to include a trailing slash in the base URL when there is a path included.

  >>> urljoin("http://marathon.mesos/service/marathon", "v2/apps")
  'http://marathon.mesos/service/v2/apps' # This is wrong. Include trailing slash to fix.
@degemer degemer added this to the 5.9.0 milestone Jun 23, 2016
@degemer degemer self-assigned this Jun 23, 2016
@degemer
Copy link
Member

degemer commented Jun 23, 2016

Thanks for the PR @ensonik!
Could you add a special case when there is no trailing slash in the URL ? (and add a trailing slash in the code)
Something like:

if url[-1] != '/':
    url += '/'

@degemer
Copy link
Member

degemer commented Jun 23, 2016

Fix #2612

@degemer
Copy link
Member

degemer commented Jul 26, 2016

Sorry, my comment is probably not needed, merging.
Thanks again for your contribution @ensonik !

@degemer degemer merged commit 8dd7f37 into DataDog:master Jul 26, 2016
degemer added a commit that referenced this pull request Jul 28, 2016
* master: (119 commits)
  [core] remove noisy logs (#2715)
  run multiple instances of pylint (#2716)
  [packaging] Release 5.8.5 (#2712)
  [changelog][5.8.5] Add notes on packaging changes (#2710)
  [changelog] Update 5.8.5 with python upgrade
  be more specific when logging ssh errors (#2708)
  [marathon] allow base_url with path (#2620)
  [changelog] Update 5.8.5
  add issue and pr templates
  [sdk] minor tweaks - sdk env detection, check location (#2694)
  [http_check] log exceptions 🔊
  use 0.0.0.0 as server address when non_local_traffic is passed (#2691)
  [elastic] tag stats metric with the node name 🏷 (#2696)
  [openstack] moving proxy logic to AgentCheck, for maintainability. Fixing typos.
  [changelog] 5.8.5 draft
  [tests] lower flakiness of test_no_parallelism (#2690)
  [core] don't use docker hostname if it's a EC2 one (#2661)
  [haproxy] Fix `KeyError` when an unknown status is found (#2681)
  [changelog] Fix md links
  [jenkins] Deprecate check (#2688)
  ...
truthbk pushed a commit that referenced this pull request Aug 10, 2016
As tested in the REPL, the fix will properly join base url's and paths:
  >>> urljoin("http://marathon.mesos", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos/service/marathon/", "v2/apps")
  'http://marathon.mesos/service/marathon/v2/apps'

  >>> urljoin("http://marathon.mesos", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos/", "v2/apps")
  'http://marathon.mesos/v2/apps'

  >>> urljoin("http://marathon.mesos:8080", "v2/apps")
  'http://marathon.mesos:8080/v2/apps'

  >>> urljoin("http://marathon.mesos:8080/", "v2/apps")
  'http://marathon.mesos:8080/v2/apps'

Care needs to be taken to include a trailing slash in the base URL when there is a path included.

  >>> urljoin("http://marathon.mesos/service/marathon", "v2/apps")
  'http://marathon.mesos/service/v2/apps' # This is wrong. Include trailing slash to fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants