Maduong/bugfix/282569 the cli should look for a compose file anywhere in the folder (not just at the root) #1183
Conversation
|
Hi @thegalah, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
| CWD = 0 | ||
| for walk in os.walk(_gitroot()): | ||
| for file_name in walk[FILE_NAMES]: | ||
| if file_name == filename: |
There was a problem hiding this comment.
this should probably be case-insensitive (file_name.lower() == file_name.lower())
| for file_name in walk[FILE_NAMES]: | ||
| if file_name == filename: | ||
| return walk[CWD] + '/' + file_name | ||
| return False |
| docker_compose_test_file = _get_filepath_in_current_git_repo(DOCKER_COMPOSE_TEST_FILE) | ||
|
|
||
| if not docker_compose_file: | ||
| raise CLIError('Docker compose file "{}" was not found.'.format(DOCKER_COMPOSE_FILE)) | ||
| with open(docker_compose_file, 'r') as f: | ||
| compose_data = yaml.load(f) | ||
| if 'version' not in compose_data.keys(): |
There was a problem hiding this comment.
checking for version and is exactly the same for both files - consider refactoring (e.g. def ensure_version(file_name, expected_version)
| @@ -192,49 +197,73 @@ def list_releases(name, resource_group_name): | |||
| json_request = req.json() | |||
| return json_request | |||
|
|
|||
| def _gitroot(): | |||
| """ | |||
| returns the absolute path of the repository root | |||
| @@ -215,9 +215,9 @@ def _get_filepath_in_current_git_repo(filename): | |||
| CWD = 0 | |||
| for walk in os.walk(_gitroot()): | |||
| for file_name in walk[FILE_NAMES]: | |||
| if file_name == filename: | |||
| if file_name.lower(file_name) == file_name.lower(filename): | |||
There was a problem hiding this comment.
This with throw - .lower() does not take any parameters. Make sure you run the code locally before submitting.
| if docker_compose_test_file: | ||
| _ensure_version(docker_compose_test_file, DOCKER_COMPOSE_EXPECTED_VERSION) | ||
|
|
||
| def _ensure_version(absolute_filepath, expected_version): |
| """ | ||
| FILE_NAMES = 2 | ||
| CWD = 0 | ||
| for walk in os.walk(_gitroot()): |
There was a problem hiding this comment.
Since os.walk() returns a 3-tuple, expand the tuple first then just use those variables instead of indexing into the tuple each time you use it:
So
for dirpath, _, filenames in os.walk(_gitroot()):
...
I use underscore _ above because it doesn't look like you use the second element in the tuple.
There was a problem hiding this comment.
_ python convention ref: http://stackoverflow.com/a/5893186 (couldn't find an official ref.)
There was a problem hiding this comment.
Yeah it was failing the pylinter. I guess this is the proper way to do it.
Resolved
| for walk in os.walk(_gitroot()): | ||
| for file_name in walk[FILE_NAMES]: | ||
| if file_name.lower(file_name) == file_name.lower(filename): | ||
| return walk[CWD] + '/' + file_name |
There was a problem hiding this comment.
os.path.join(dirpath, name) instead to be platform independent.
| CWD = 0 | ||
| for walk in os.walk(_gitroot()): | ||
| for file_name in walk[FILE_NAMES]: | ||
| if file_name.lower(file_name) == file_name.lower(filename): |
There was a problem hiding this comment.
Suggest more descriptive name for the variable filename.
At a glance, file_name.lower(file_name) == file_name.lower(filename) looks like the same variables are being compared because the names are so similar.
| CWD = 0 | ||
| for walk in os.walk(_gitroot()): | ||
| for file_name in walk[FILE_NAMES]: | ||
| if file_name.lower() == filename.lower(): |
There was a problem hiding this comment.
Suggest more descriptive name for the variable filename.
At a glance, file_name.lower() == filename.lower() looks like the same variables are being compared because the names are so similar.
| for walk in os.walk(_gitroot()): | ||
| for file_name in walk[FILE_NAMES]: | ||
| if file_name.lower() == filename.lower(): | ||
| return walk[CWD] + '/' + file_name |
There was a problem hiding this comment.
os.path.join(dirpath, name) instead to be platform independent.
| try: | ||
| base = check_output(['git', 'rev-parse', '--show-toplevel']) | ||
| except: | ||
| raise IOError('Current working directory is not a git repository') |
There was a problem hiding this comment.
All exceptions are being caught but exceptions could be raised for different reasons.
For example, the way the code is right now, if I don't have 'git' installed on my machine an exception will be raised and then the code will raise IOError('Current working directory is not a git repository') which isn't true.
Just as a suggestion, you could:
1st. catch OSError separately which will be thrown if git isn't installed.
2nd. catch CalledProcessError which will be thrown if git returned an error code.
https://docs.python.org/2/library/subprocess.html#subprocess.check_output
https://docs.python.org/2/library/subprocess.html#exceptions
There was a problem hiding this comment.
Resolved. I've thrown CLIErrors for both types of caught exceptions now
| base = check_output(['git', 'rev-parse', '--show-toplevel']) | ||
| except OSError: | ||
| raise CLIError('Git is not currently installed.') | ||
| except CalledProcessError: |
There was a problem hiding this comment.
Need to add CalledProcessError to the from subprocess import ... line at the very top.
…9-The-CLI-should-look-for-a-compose-file-anywhere-in-the-folder-(not-just-at-the-root) * Azure/master: add/change registry and remote branch parameters (Azure#1179)
Verifies docker-compose and docker file exist somewhere in the repo, not just the root