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

Enable broker support on Linux for WSL #766

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

Enable broker support on Linux for WSL #766

wants to merge 10 commits into from

Conversation

DharshanBJ
Copy link
Contributor

No description provided.

@DharshanBJ DharshanBJ requested a review from a team as a code owner November 7, 2024 00:34
@DharshanBJ
Copy link
Contributor Author

/azp run MSAL-Python-SDL-CI

fengga
fengga reviewed Nov 7, 2024
View reviewed changes
setup.cfg Show resolved Hide resolved
fengga
fengga approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@fengga fengga left a comment

Choose a reason for hiding this comment

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

Thanks, this PR looks good to me. And please make sure get an approval from Ray.

rayluo
rayluo reviewed Nov 8, 2024
View reviewed changes
tests/test_e2e.py
@@ -200,6 +200,7 @@ def _build_app(cls,
http_client=http_client or MinimalHttpClient(),
enable_broker_on_windows=broker_available,
enable_broker_on_mac=broker_available,
enable_broker_on_linux=broker_available,
Copy link
Collaborator

Choose a reason for hiding this comment

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

praise: Thanks for finding this test file!

suggestion: If you haven't already, please also do python -m unittest tests.test_e2e and make sure we won't run into any issue. Note that you will need to setup an .env first, whose content is described at the beginning of this test_e2e.py file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't support POP and ssh-cert flows for linux yet, tested the other e2e flows.

rayluo
rayluo reviewed Nov 8, 2024
View reviewed changes
tests/broker-test.py Outdated Show resolved Hide resolved
rayluo
rayluo reviewed Nov 8, 2024
View reviewed changes
sample/interactive_sample.py Outdated Show resolved Hide resolved
rayluo
rayluo reviewed Nov 8, 2024
View reviewed changes
msal/application.py Outdated Show resolved Hide resolved
rayluo
rayluo reviewed Nov 8, 2024
View reviewed changes
msal/application.py Outdated Show resolved Hide resolved
rayluo
rayluo requested changes Nov 8, 2024
View reviewed changes
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for a clean PR! Implementation wise, it looks good. I added some suggestions above, mostly in terms of our workflow. Please make corresponding changes and then wait for the PyMsalRuntime release.

rayluo
rayluo requested changes Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

We will also need to change the precise dependency version error message here and a approximate version hint there

@DharshanBJ
Copy link
Contributor Author

We will also need to change the precise dependency version error message here and a approximate version hint there

updated

@DharshanBJ DharshanBJ changed the title Enable broker support on Linux Enable broker support on Linux for WSL Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvaneck pvaneck pvaneck left review comments

@rayluo rayluo rayluo requested changes

@fengga fengga fengga approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DharshanBJ @rayluo @pvaneck @fengga