Skip to content

{Packaging} Add msal-extensions dependency #19910

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

Merged
jiasli merged 2 commits into from
Oct 18, 2021
Merged

{Packaging} Add msal-extensions dependency #19910

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b696ac0
Add msal-extensions dependency
jiasli Oct 16, 2021
188298b
upper bound
jiasli Oct 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/azure-cli-core/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,14 @@
'humanfriendly>=4.7,<10.0',
'jmespath',
'knack~=0.8.2',
'msal-extensions>=0.3.0',
Copy link
Member

@rayluo rayluo Oct 16, 2021

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?

Copy link
Member Author

@jiasli jiasli Oct 16, 2021

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. 😉

Copy link
Member

@rayluo rayluo Oct 16, 2021

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.

Copy link
Member Author

@jiasli jiasli Oct 16, 2021

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.

Copy link
Member

@rayluo rayluo Oct 16, 2021

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:

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.

'msal>=1.15.0,<2.0.0',
Copy link
Member Author

@jiasli jiasli Oct 16, 2021

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.

Copy link
Member

@rayluo rayluo Oct 16, 2021

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. :-)

Copy link
Member Author

@jiasli jiasli Oct 18, 2021

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.

'paramiko>=2.0.8,<3.0.0',
'pkginfo>=1.5.0.1',
'PyJWT>=2.1.0',
'pyopenssl>=17.1.0', # https://github.com/pyca/pyopenssl/pull/612
'requests[socks]~=2.25.1',
'urllib3[secure]>=1.26.5',
'urllib3[secure]>=1.26.5'
]

# dependencies for specific OSes
Expand Down
3 changes: 2 additions & 1 deletion src/azure-cli/requirements.py3.Darwin.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ azure-mgmt-sqlvirtualmachine==1.0.0b1
azure-mgmt-storage==19.0.0
azure-mgmt-synapse==2.0.0
azure-mgmt-trafficmanager==0.51.0
azure-multiapi-storage==0.7.0
azure-mgmt-web==4.0.0
azure-multiapi-storage==0.7.0
azure-nspkg==3.0.2
azure-storage-common==1.4.2
azure-synapse-accesscontrol==0.5.0
Expand All @@ -110,6 +110,7 @@ jmespath==0.9.5
jsondiff==1.2.0
knack==0.8.2
MarkupSafe==1.1.1
msal-extensions==0.3.0
msal==1.15.0
msrest==0.6.21
msrestazure==0.6.3
Expand Down
1 change: 1 addition & 0 deletions src/azure-cli/requirements.py3.Linux.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ jmespath==0.9.5
jsondiff==1.2.0
knack==0.8.2
MarkupSafe==1.1.1
msal-extensions==0.3.0
msal==1.15.0
msrest==0.6.21
msrestazure==0.6.3
Expand Down
1 change: 1 addition & 0 deletions src/azure-cli/requirements.py3.windows.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ jmespath==0.9.5
jsondiff==1.2.0
knack==0.8.2
MarkupSafe==1.1.1
msal-extensions==0.3.0
msal==1.15.0
Comment on lines +112 to 113
Copy link
Member Author

@jiasli jiasli Oct 16, 2021

Choose a reason for hiding this comment

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

- < =

msrest==0.6.21
msrestazure==0.6.3
Expand Down