Skip to content
This repository was archived by the owner on Jan 19, 2018. It is now read-only.

Conversation

@surajssd
Copy link
Collaborator

Added a staticmethod in utils.Utils named get_abspath which abstracts the use of os.path.join(Utils.getRoot, path)

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

3 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?


@staticmethod
def get_abspath(path):
return os.path.join(Utils.getRoot(), path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should add .lstrip('/') to path here, as it should be always called with lstrip('/')

getRoot returns '/host' or '/'
with leading '/' in second argument os.path.join cuts first part in path

>>> os.path.join("/host","/foo/bar")
'/foo/bar'

>>> os.path.join("/host","foo/bar")
'/host/foo/bar'

@surajssd surajssd force-pushed the abstract-get_abspath branch from bd81b13 to df2885d Compare April 20, 2016 10:39
@surajssd
Copy link
Collaborator Author

@kadel done!

@kadel
Copy link
Collaborator

kadel commented Apr 20, 2016

#dotests


@staticmethod
def get_abspath(path):
return os.path.join(Utils.getRoot(), path.lstrip('/'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about what this function is doing if you don't mind.

@dustymabe
Copy link
Contributor

kube test failed due to yum error reaching repos. Trying again.

@dustymabe
Copy link
Contributor

#dotestkubernetes

Added a staticmethod in utils.Utils named get_real_abspath
which abstracts the use of os.path.join(Utils.getRoot, path)
@surajssd surajssd force-pushed the abstract-get_abspath branch from df2885d to 001b3e4 Compare April 20, 2016 17:53
@surajssd
Copy link
Collaborator Author

surajssd commented Apr 20, 2016

@dustymabe done as you said also rebased!

@dustymabe
Copy link
Contributor

#dotests

@dustymabe
Copy link
Contributor

LGTM - @cdrage.. do you mind spot checking this before merge?

args.cli_answers[item] = getattr(args, item)

lock = LockFile(os.path.join(Utils.getRoot(), LOCK_FILE))
lock = LockFile(Utils.get_real_abspath(LOCK_FILE))
Copy link
Member

Choose a reason for hiding this comment

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

I'll have to rebase this on my PR on removing the lockfile, but that's fine :)

@cdrage
Copy link
Member

cdrage commented Apr 20, 2016

Tests pass locally, code looks a-okay! LGTM 👍 Merge when ready

@dustymabe
Copy link
Contributor

#dotestopenshift

@dustymabe dustymabe merged commit cef5ec5 into projectatomic:master Apr 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants