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 base url #34

Merged
merged 4 commits into from
Mar 22, 2020
Merged

Fix base url #34

merged 4 commits into from
Mar 22, 2020

Conversation

gal-berger
Copy link
Contributor

@gal-berger gal-berger commented Mar 19, 2020

Status

Ready

Fixes

https://github.com/demisto/etc/issues/22837

Description

DEMISTO_BASE_URL slash inconsistency

Must have

  • Unit Test or Example Code
  • Changelog entry

Copy link

@DeanArbel DeanArbel left a comment

Choose a reason for hiding this comment

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

Did you test this in demisto-sdk?

CHANGELOG.md Outdated
@@ -4,7 +4,8 @@

[1]: https://pypi.org/project/demisto-py/#history
## 2.0.9
Improved error message when missing authentication parameters.
* Improved error message when missing authentication parameters.
* fix base_url slash inconsistency

Choose a reason for hiding this comment

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

It's configuration.host and not base_url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, fixing it.

Copy link
Contributor Author

@gal-berger gal-berger Mar 19, 2020

Choose a reason for hiding this comment

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

I tested generic_request_func directly with the next options:

  1. base/ + /path

2.base + / path

  1. base + path

4.base/ + path

@@ -4,7 +4,8 @@

[1]: https://pypi.org/project/demisto-py/#history
## 2.0.9
Improved error message when missing authentication parameters.
* Improved error message when missing authentication parameters.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the change log entries to a new version: 2.0.10. 2.0.9 was released alread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Collaborator

@amshamah419 amshamah419 left a comment

Choose a reason for hiding this comment

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

Just commit the changes to the changelog. Otherwise, we walked through this and it works well.

CHANGELOG.md Outdated
@@ -3,8 +3,11 @@
[PyPI History][1]

[1]: https://pypi.org/project/demisto-py/#history
## 2.0.10
* fix configuration.host slash inconsistency
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* fix configuration.host slash inconsistency
* Enabled host and path parameters to function with trailing or leading slashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

@DeanArbel DeanArbel left a comment

Choose a reason for hiding this comment

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

Good job. 👍

@gal-berger gal-berger merged commit 95655a4 into master Mar 22, 2020
@gal-berger gal-berger deleted the fix_base_url branch March 22, 2020 11:08
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.

None yet

4 participants