Skip to content
This repository was archived by the owner on May 6, 2020. It is now read-only.

Conversation

@sboeuf
Copy link
Contributor

@sboeuf sboeuf commented May 2, 2017

Some paths provided in the cc-proxy.service and cc-proxy.socket were both relying on legacy path names from cc-oci-runtime. This directly led the cc-proxy service to be not started as expected when the proxy socket was used.

Some paths provided in the cc-proxy.service and cc-proxy.socket were
both relying on legacy path names from cc-oci-runtime. This directly
led the cc-proxy service to be not started as expected when the proxy
socket was used.

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
@sboeuf sboeuf requested a review from dlespiau May 2, 2017 21:32
@coveralls
Copy link

coveralls commented May 2, 2017

Coverage Status

Coverage increased (+0.4%) to 71.053% when pulling bc3fe0a on sboeuf/fix_service into 398fd8b on master.


[Service]
ExecStart=@libexecdir@/cc-proxy
ExecStart=@libexecdir@/clearcontainers/cc-proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

can we standardise on clear-containers? see clearcontainers/runtime#85

@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage remained the same at 70.614% when pulling 1d02d3c on sboeuf/fix_service into 398fd8b on master.

Makefile Outdated

SOURCES := $(shell find . 2>&1 | grep -E '.*\.(c|h|go)$$')
PROXY_SOCKET := $(LOCALSTATEDIR)/run/clearcontainers/proxy.sock
PROXY_SOCKET := $(LOCALSTATEDIR)/run/$(CLEARCONTAINERS_SUFFIX)/proxy.sock
Copy link
Contributor

Choose a reason for hiding this comment

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

s/_SUFFIX// that's not really a suffix DIR maybe? or just have clear-containers everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're fine with hardcoding this "clear-containers" everywhere, I am fine with it.
@jodh-intel any thought on this ?

Choose a reason for hiding this comment

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

I'd rename to something like CLEAR_CONTAINERS_DIR for legibility.

@dlespiau
Copy link
Contributor

dlespiau commented May 3, 2017

I'm absolutely fine with this

Because it has been decided that clear-containers will be cleaner and
because we want to be consistent accross all projects, let's replace
"clearcontainers" with "clear-containers".

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
@sboeuf sboeuf force-pushed the sboeuf/fix_service branch from 1d02d3c to 8870d77 Compare May 3, 2017 16:04
@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage remained the same at 70.614% when pulling 8870d77 on sboeuf/fix_service into 398fd8b on master.

@sboeuf
Copy link
Contributor Author

sboeuf commented May 3, 2017

@dlespiau Does that sound good now ?

@dlespiau
Copy link
Contributor

dlespiau commented May 3, 2017

I would squash the two commits together, you're changing the same strings twice in a row! but let's move on :)

lgtm

Approved with PullApprove

@dlespiau dlespiau merged commit 4f87d2a into master May 3, 2017
@dlespiau dlespiau deleted the sboeuf/fix_service branch May 3, 2017 16:31
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