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

Feature/docker refactorings #794

Merged
merged 16 commits into from
Jun 16, 2021

Conversation

alexadhy
Copy link
Contributor

@alexadhy alexadhy commented Jun 4, 2021

Did you run make format && make check? yes

Fixes #793

Changes:

  • Refactor docker image according to the issue

How to test this PR:

  • See README on how to run skywire-visor

@jdknives
Copy link
Member

jdknives commented Jun 8, 2021

@alexadhy running without volume defined does not work for me:

log.txt

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2021

CLA assistant check
All committers have signed the CLA.

@jdknives
Copy link
Member

jdknives commented Jun 9, 2021

I am still running into the same problem as before. In addition, can we add a make target for building the docker image?

log.txt

@alexadhy
Copy link
Contributor Author

@jdknives: I think it was because you built the image with git rev-parse --abbrev-ref HEAD tag while running it with skywire:test tag, the docker image tag should be latest for master branch, test for develop branch, and the same branch name if it's other branch.

Should I change that behaviour?

@jdknives
Copy link
Member

@alexadhy works well. We should also copy over Skychat and the skysocks app to the container. Afterwards this can be merged.

Makefile Outdated
@@ -138,6 +138,9 @@ build-deploy: ## Build for deployment Docker images
${OPTS} go build ${BUILD_OPTS_DEPLOY} -o /release/apps/skysocks ./cmd/apps/skysocks
${OPTS} go build ${BUILD_OPTS_DEPLOY} -o /release/apps/skysocks-client ./cmd/apps/skysocks-client

build-docker: ## Build docker image
./ci_scripts/docker-push.sh -t develop -b
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a var here instead of hardcoding the tag? May be more helpful this way.

@jdknives jdknives merged commit 7299c7e into skycoin:develop Jun 16, 2021
@alexadhy alexadhy deleted the feature/docker-refactorings branch June 16, 2021 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants