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

Maduong/bugfix/282569 the cli should look for a compose file anywhere in the folder (not just at the root) #1183

Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
RP_URL = BASE_URL + CONTAINER_SERVICE_RESOURCE_URL + "/providers/Microsoft.Mindaro"
API_VERSION = "2016-11-01-preview"

DOCKERFILE_FILE = 'Dockerfile'
DOCKER_COMPOSE_FILE = 'docker-compose.yml'
DOCKER_COMPOSE_TEST_FILE = 'docker-compose.test.yml'
DOCKER_COMPOSE_EXPECTED_VERSION = '2'

def add_release(
name,
resource_group_name,
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

nit: Gets the abs....

"""
try:
base = check_output(['git', 'rev-parse', '--show-toplevel'])
except:
raise IOError('Current working directory is not a git repository')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved. I've thrown CLIErrors for both types of caught exceptions now

return base.decode('utf-8').strip()

def _get_filepath_in_current_git_repo(filename):
"""
retrieves the full path of the first file in the git repo that matches filename
"""
FILE_NAMES = 2
CWD = 0
for walk in os.walk(_gitroot()):
Copy link
Member

Choose a reason for hiding this comment

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

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.

https://docs.python.org/2/library/os.html#os.walk

Copy link
Member

@derekbekoe derekbekoe Oct 28, 2016

Choose a reason for hiding this comment

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

_ python convention ref: http://stackoverflow.com/a/5893186 (couldn't find an official ref.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was failing the pylinter. I guess this is the proper way to do it.
Resolved

for file_name in walk[FILE_NAMES]:
if file_name == filename:
Copy link

Choose a reason for hiding this comment

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

this should probably be case-insensitive (file_name.lower() == file_name.lower())

return walk[CWD] + '/' + file_name
Copy link
Member

Choose a reason for hiding this comment

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

os.path.join(dirpath, name) instead to be platform independent.

https://docs.python.org/2/library/os.path.html#os.path.join

Copy link
Member

Choose a reason for hiding this comment

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

os.path.join(dirpath, name) instead to be platform independent.

https://docs.python.org/2/library/os.path.html#os.path.join

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

return False
Copy link

Choose a reason for hiding this comment

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

Return None


def _ensure_docker_compose():
"""
1. Raises an error if there is no docker_compose_file present.
2. Raises an error if the version specified in the docker_compose_file is not
docker_compose_version.
3. Raises an error if docker_compose_test_file has a version other than docker_compose_version.
"""
docker_compose_file = 'docker-compose.yml'
docker_compose_test_file = 'docker-compose.test.yml'
docker_compose_expected_version = '2'
if not os.path.isfile(docker_compose_file):
raise CLIError('Docker compose file "{}" was not found.'.format(docker_compose_file))
docker_compose_file = _get_filepath_in_current_git_repo(DOCKER_COMPOSE_FILE)
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():
Copy link

Choose a reason for hiding this comment

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

checking for version and is exactly the same for both files - consider refactoring (e.g. def ensure_version(file_name, expected_version)

raise CLIError(
'Docker compose file "{}" is missing version information.'.format(
docker_compose_file))
if not docker_compose_expected_version in compose_data['version']:
DOCKER_COMPOSE_FILE))
if not DOCKER_COMPOSE_EXPECTED_VERSION in compose_data['version']:
raise CLIError(
'Docker compose file "{}" has incorrect version. \
Only version "{}" is supported.'.format(
docker_compose_file,
docker_compose_expected_version))
if os.path.isfile(docker_compose_test_file):
DOCKER_COMPOSE_FILE,
DOCKER_COMPOSE_EXPECTED_VERSION))

if docker_compose_test_file:
with open(docker_compose_test_file, 'r') as f:
compose_data = yaml.load(f)
if 'version' not in compose_data.keys():
raise CLIError('Docker compose file "{}" is missing version information.'.format(
docker_compose_test_file))
if not docker_compose_expected_version in compose_data['version']:
if not DOCKER_COMPOSE_EXPECTED_VERSION in compose_data['version']:
raise CLIError(
'Docker compose file "{}" has incorrect version. \
Only version "{}" is supported.'.format(
docker_compose_test_file,
docker_compose_expected_version))
DOCKER_COMPOSE_EXPECTED_VERSION))

def _ensure_dockerfile():
"""
1. Raises an error if there is no dockerfile present.
"""
dockerfile_file = 'Dockerfile'
if not os.path.isfile(dockerfile_file):
dockerfile_file = _get_filepath_in_current_git_repo(DOCKERFILE_FILE)

if not dockerfile_file:
raise CLIError('Docker file "{}" was not found.'.format(dockerfile_file))

def _get_remote_url():
Expand Down