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

Drop python 2 and 3.5 #174

Merged
merged 5 commits into from
Feb 20, 2020
Merged

Drop python 2 and 3.5 #174

merged 5 commits into from
Feb 20, 2020

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Feb 17, 2020

Drop python 2 and 3.5 as Azure CLI has dropped them: Azure/azure-cli#11363

@jiasli jiasli self-assigned this Feb 17, 2020
@jiasli jiasli requested a review from yonzhan February 18, 2020 01:58
C901
C901,
W503,
W504
Copy link
Member Author

@jiasli jiasli Feb 18, 2020

Choose a reason for hiding this comment

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

New entries are added to mute newer versions of flake8.

According to pycodestyle

E741: do not use variables named ‘l’, ‘O’, or ‘I’
E722: do not use bare except, specify exception instead
W503: line break before binary operator
W504: line break after binary operator

six==1.14.0
tabulate==0.8.6
vcrpy==4.0.2
pytest==5.3.5
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated using pip freeze --requirement requirements.txt

@@ -39,12 +39,11 @@
'Intended Audience :: Developers',
'Intended Audience :: System Administrators',
'Programming Language :: Python',
'Programming Language :: Python :: 2',
'Programming Language :: Python :: 2.7',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.5',
Copy link
Member Author

@jiasli jiasli Feb 18, 2020

Choose a reason for hiding this comment

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

3.5 tag is still retained, but not tested in CI.

self.assertFalse(bool(file_mode & stat.S_IXGRP))
self.assertFalse(bool(file_mode & stat.S_IROTH))
self.assertFalse(bool(file_mode & stat.S_IWOTH))
self.assertFalse(bool(file_mode & stat.S_IXOTH))
Copy link
Member Author

Choose a reason for hiding this comment

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

Windows doesn't have Linux-like user/group/other concepts for stat. rwx is always the same for all scopes.

Choose a reason for hiding this comment

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

image

>>> import sys
>>> sys.version
'3.8.0 (tags/v3.8.0:fa919fd, Oct 14 2019, 19:37:50) [MSC v.1916 64 bit (AMD64)]'

As far as I understand from the doc, the limitation only takes effect on os.chmod, however, os.stat works for me. The doc didn't mention that os.stat attributes won't be populated on Windows.

I agreed on file mode checking across different OS platform, but if file mode checking differentiate on Windows, should we add additional support?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also didn't find the doc for os.stat's behavior on Windows.

st_mode=33206 is the default for a Windows file, which is ‭1000000 110 110 110‬ in binary. You can see rwx is the same 110 for different scopes. However you change it, it keeps this pattern. For example, if you change the file to read-only via os.chmod('{}', stat.S_IRUSR), os.stat is 33060 which is ‭1000000 100 100 100‬ in binary.

Checking permission on Windows requires win32security according to https://stackoverflow.com/a/12168268/2199657. This will over complicate the test. Since we only save the config file under ~/.azure, we can assume Windows behaves correctly unless the system is compromised, but of course this is out of the scope of the app itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe you are running this command on Cygwin which has additional file management logic. Running os.chmod and os.stat on bare Windows will repro my observation.

Choose a reason for hiding this comment

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

No, on CMD

Copy link

@fengzhou-msft fengzhou-msft left a comment

Choose a reason for hiding this comment

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

LGTM, installed and verified in local environment.

@jiasli jiasli merged commit 219399c into microsoft:master Feb 20, 2020
@jiasli jiasli deleted the drop-python2 branch March 17, 2020 04:39
@jiasli jiasli mentioned this pull request Mar 20, 2020
@jiasli jiasli mentioned this pull request Jan 29, 2021
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.

3 participants