-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
{Packaging} Add msal-extensions
dependency
#19910
Conversation
msal-extensions==0.3.0 | ||
msal==1.15.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
< =
src/azure-cli-core/setup.py
Outdated
'msal-extensions>=0.3.0', | ||
'msal>=1.15.0,<2.0.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ASCII, -
appears before >
, so alphabetically msal-extensions>=0.3.0
is smaller than msal>=1.15.0,<2.0.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You folks sort dependency alphabetically? Interesting. I am not aware that practice, but it sounds good. But I would assume the sorting should only compare the package name (i.e. msal
< msal-extensions
), not the entire versioning condition (i.e. msal>...
< msal-extensions...
).
Just my random thoughts. You can ignore this comment. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In deed, sorting by package name is better, but is is hard to do with an IDE.
As the dependencies of Azure CLI are too many, not soring them will frequently cause merging conflicts.
src/azure-cli-core/setup.py
Outdated
@@ -50,13 +50,14 @@ | |||
'humanfriendly>=4.7,<10.0', | |||
'jmespath', | |||
'knack~=0.8.2', | |||
'msal-extensions>=0.3.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you consider adding an upper bound such as msal-extensions>=0.3.0,<1
, or strictly speaking, msal-extensions>=0.3.0,<0.4
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends on whether msal-extensions
will make a breaking change in 1.0 or 0.4. I can't predict the future. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. My point was you would want to at least use some upper bound, rather than no upper bound at all. I think you can go with extensions>=0.3.0,<0.4
, for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, recently we deliberately removed upper bound for some libraries like cryptography
(#19639) so that users can always use the latest versions of dependencies, rather than waiting for us to bump it. Since pip
is being more strict than before, setting a wider version range helps eliminate dependency conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msal
and msal-extensions
use Semantic Versioning. And the whole point of Semantic Versioning (i.e. Why Semantic Versioning) is to be able to:
- precisely setting a lower bound (which is also the idea discussed in {Packaging} Bump cryptography from 2.8 to 3.3.2 #15687 (comment));
- confidently setting an upper bound for a future version yet without worrying about breaking change.
So, I would still suggest you to properly set both lower and upper bounds for msal
and msal-extensions
.
cryptography
is a different story. It does not use SemVer, so its practice is vastly different. It is off-topic in this context. We can follow up offline separately.
Packaging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Even though
azure-identity
depends onmsal-extensions
andazure-cli
depends onazure-identity
:azure-cli
should declare the direct dependency onmsal-extensions
asmsal-extensions
is directly used byazure-cli
.