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

only add Authorization header if token exists #6

Merged

Conversation

reimer-atb
Copy link
Contributor

@reimer-atb reimer-atb commented Sep 29, 2022

This patch makes the database config "token" optional. Users can either set it to an empty string or omit the token completely. This is useful for DB-APIs that do not require authentication, and therefore no token.

Also closes #7

It is necessary to pin the werkzeug dependency to version 2.0.*. Otherwise, service-discovery will fail to start with:

ImportError: cannot import name 'parse_rule' from 'werkzeug.routing'

See python-restx/flask-restx#460

service-discovery probably should update its dependencies as soon as there is an updated version of flask-restx available.

This patch makes the database config "token" optional.
Users can either set it to an empty string or omit the token completely.
This is useful for DB-APIs that do not require authentication, and
therefore no token.
Copy link
Contributor

@dberrocal-git dberrocal-git left a comment

Choose a reason for hiding this comment

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

There is still no update available to fix the bug with werkzeug, therefore LGTM.

@reimer-atb
Copy link
Contributor Author

@dabm-git The changes from this PR seem to be included in #9 too? Can this PR still be merged or has it been superseeded by #9?

Do we require additional approval from @zaalizadeh / @Zakieh2020 ?

@Zakieh2020
Copy link

Zakieh2020 commented Nov 16, 2022

@dabm-git The changes from this PR seem to be included in #9 too? Can this PR still be merged or has it been superseeded by #9?

Do we require additional approval from @zaalizadeh / @Zakieh2020 ?

Regarding the second question,
@dabm-git has implemented this module and, his approval will be the main approve for this repository

@reimer-atb
Copy link
Contributor Author

Regarding the second question, @dabm-git has implemented this module and, his approval will be the main approve for this repository

Ok. Since we already have @dabm-git 's approval I suggest to merge this so that we get a working version of service-discovery - and update / handle #9 subsequently.

@deanone @schlotze if you agree, could you do the merge?

@schlotze
Copy link
Contributor

@deanone @schlotze if you agree, could you do the merge?

Yes, will do

@schlotze schlotze merged commit c5a6a2b into main Nov 16, 2022
@schlotze schlotze deleted the fix/allow-requests-to-db-api-that-does-not-require-token branch November 16, 2022 12:36
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.

Running service-discovery fails because of incompatibility between dependencies flask-restx and werkzeug
5 participants