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

Add a check for linux version consistency #5747

Closed
wants to merge 9 commits into from
Closed

Add a check for linux version consistency #5747

wants to merge 9 commits into from

Conversation

yorek
Copy link
Contributor

@yorek yorek commented Mar 7, 2018

Made all the changes requested in #5675

@yorek yorek requested a review from derekbekoe March 7, 2018 04:15
@promptws
Copy link

promptws commented Mar 7, 2018

View a preview at https://prompt.ws/r/Azure/azure-cli/5747
This is an experimental preview for @microsoft users.

derekbekoe
derekbekoe suggested changes Mar 7, 2018
View reviewed changes
Copy link
Member

@derekbekoe derekbekoe 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 the PR.
It looks good. Just a few comments.

Later, I will do some local testing of this PR against some different distros etc. to verify further then we can merge.

grep Outdated
@@ -0,0 +1,55 @@
DEBUG: Command arguments: ['extension', 'add', '-n', 'azure-cli-iot-ext', '--debug', 'Linux distro check:']
Copy link
Member

Choose a reason for hiding this comment

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

Remove this file from the PR.

output.txt Outdated
@@ -0,0 +1,3495 @@
DEBUG: Command arguments: ['extension', 'add', '-n', 'azure-cli-iot-ext', '--debug']
Copy link
Member

Choose a reason for hiding this comment

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

Remove this file from the PR.

src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/__init__.py Outdated
@@ -29,7 +29,7 @@ def ext_add_has_confirmed(command_args):
def transform_extension_list_available(results):
return [OrderedDict([('Name', r)]) for r in results]

def validate_extension_add(namespace):
def validate_extension_add(namespace):
Copy link
Member

Choose a reason for hiding this comment

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

None of the changes in this file are needed so please revert.

src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/custom.py Outdated
logger.debug('Linux distro check: Found in list file: %s', stored_linux_dist_name)

current_linux_dist_name = platform.linux_distribution()[2]
logger.debug('Linux distro check: Reported by API: %s', current_linux_dist_name)
Copy link
Member

Choose a reason for hiding this comment

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

Put the above 4 lines indented under the above with statement.
package_source would be defined in the scope of the with statement so the other code should be under it.

src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/custom.py Outdated

except Exception as err:
current_linux_dist_name = ""
stored_linux_dist_name = ""
Copy link
Member

Choose a reason for hiding this comment

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

Instead of empty string, I'd prefer None.

src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/custom.py Outdated
@@ -134,8 +138,10 @@ def _add_whl_ext(source, ext_sha256=None, pip_extra_index_urls=None, pip_proxy=N
logger.debug(traceback.format_exc())
raise CLIError('The extension is invalid. Use --debug for more information.')
except CLIError as e:
raise e
raise e
Copy link
Member

Choose a reason for hiding this comment

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

These trailing spaces would cause CI to fail.

@yorek
Copy link
Contributor Author

yorek commented Mar 7, 2018

Applied all the requested changes :)

@yorek
Copy link
Contributor Author

yorek commented Mar 7, 2018

And cleaned up last unneeded changes. Now everything is just in custom.py

@derekbekoe
Copy link
Member

Thanks.
One more thing I noticed that would fail on CI:

In setup.py, change the version to 0.0.10.
Then in HISTORY.rst, add an entry for the new version with one line describing the change.
Both of these files can be found in the azure-cli-extension directory.

@yorek
Copy link
Contributor Author

yorek commented Mar 7, 2018

Done :)

@derekbekoe
Copy link
Member

@yorek Can you rebase your changes on the latest from dev branch? The PR was created before the prev. release so needs to be rebased.

Also, please take a look at these failures:

Run flake8.
./src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/custom.py:269:1: E302 expected 2 blank lines, found 1
./src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/custom.py:275:50: W291 trailing whitespace
./src/command_modules/azure-cli-extension/azure/cli/command_modules/extension/custom.py:285:30: W291 trailing whitespace
1     E302 expected 2 blank lines, found 1
2     W291 trailing whitespace

@yorek
Copy link
Contributor Author

yorek commented Mar 7, 2018

Will do. Interesting that pylint did not detect that problems

@derekbekoe derekbekoe added Extensions `az extension` commands or extension infrastructure Packaging/Debian labels Mar 7, 2018
@derekbekoe derekbekoe added this to the Sprint 33 milestone Mar 7, 2018
@yorek
Copy link
Contributor Author

yorek commented Mar 7, 2018

Rebasing of "dev" done

@yorek yorek mentioned this pull request Mar 7, 2018
Merged
@derekbekoe derekbekoe closed this Mar 7, 2018
@haroldrandom haroldrandom added Extensions `az extension` commands or extension infrastructure Packaging/Debian labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekbekoe derekbekoe derekbekoe requested changes

Assignees
No one assigned
Labels
Extensions `az extension` commands or extension infrastructure Packaging/Debian
Projects
None yet
Milestone
Sprint 33
Development

Successfully merging this pull request may close these issues.
4 participants
@yorek @promptws @derekbekoe @haroldrandom