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

Update rustc and docker #970

Merged
merged 8 commits into from
Sep 23, 2018
Merged

Update rustc and docker #970

merged 8 commits into from
Sep 23, 2018

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Sep 22, 2018

No description provided.

@nhynes nhynes self-assigned this Sep 22, 2018
@nhynes nhynes requested review from willscott and kostko September 22, 2018 18:08
@codecov
Copy link

codecov bot commented Sep 22, 2018

Codecov Report

Merging #970 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #970   +/-   ##
=======================================
  Coverage   63.55%   63.55%           
=======================================
  Files          38       38           
  Lines        2445     2445           
=======================================
  Hits         1554     1554           
  Misses        709      709           
  Partials      182      182

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76b1ad0...1722e63. Read the comment docs.

@nhynes
Copy link
Contributor Author

nhynes commented Sep 22, 2018

are you serious?

screen shot 2018-09-22 at 11 26 04 am

@@ -1,6 +1,6 @@
defaults: &defaults
docker:
- image: ekiden/testing:0.2.0
- image: nhdh/testing:0.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

note: assumption is that this will change back once validated / before merged

compute/Cargo.toml Show resolved Hide resolved
docker/Makefile Outdated
@@ -0,0 +1,14 @@
.PHONY: development
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than allocating an ssh private key / github account provisioned for the dev docker file so that it can get the private fork of sgx, might be easier to hardcode a read-only github deploy key to the image - the expectation being that if you have access to this repo / image source, you also can get access to that repo.

docker/development/Dockerfile Outdated Show resolved Hide resolved
@willscott
Copy link
Contributor

Requesting a deploy-key based pull of the docker image, rather than the current make setup with external environmental expectations.

Otherwise looks good

@nhynes nhynes force-pushed the update-rust branch 2 times, most recently from 91fd955 to 04c4534 Compare September 23, 2018 00:32
@nhynes nhynes merged commit ef01c1f into master Sep 23, 2018
@nhynes nhynes deleted the update-rust branch September 23, 2018 04:31
@nhynes nhynes mentioned this pull request Sep 24, 2018
./linux/installer/bin/sgx_linux_x64_sdk_*.bin --prefix /opt && \
echo "source /opt/sgxsdk/environment" >> /root/.bashrc
COPY --from=intermediate /opt/sgxsdk /opt/sgxsdk
RUN echo "source /opt/sgxsdk/environment" >> /root/.bashrc

# install rust (versions based on those found successful in https://github.com/oasislabs/rust-sgx-sdk/blob/v1.0.1-ekiden1/dockerfile/Dockerfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this refer to -ekiden2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this comment shouldn't even exist. it implies that the working version is unpredictable, which it's not

all: $(IMAGES)

development: development/Dockerfile
docker build $@ -t $(IMG_ORG)/$@:$(IMG_VER) -f $<
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't need -f when the Dockerfile has the default name

weird, the manpages for docker-build suggests that it shouldn't even be -f deployment/Dockerfile

-f, --file=PATH/Dockerfile
Path to the Dockerfile to use. If the path is a relative path and you are
building from a local directory, then the path must be relative to that
directory
. If you are building from a remote URL pointing to either a
tarball or a Git repository, then the path must be relative to the root of
the remote context. In all cases, the file must be within the build context.
The default is Dockerfile.

(bold added)

libssl-dev libcurl4-openssl-dev

RUN mkdir /root/.ssh/
COPY deploy_key /root/.ssh/id_rsa
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ryscheng
will we have to deauthorize this key before we make this repo public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's read-only

@@ -0,0 +1,51 @@
-----BEGIN RSA PRIVATE KEY-----
MIIJKgIBAAKCAgEArF5Jh+XO+tQq79+t4GyVkO+ka7BZV0RbF03HQ6R6Tm+dPlOW
Copy link
Contributor

Choose a reason for hiding this comment

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

what key is this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a github deploy key for the oasislabs/intel-sgx repo

RUN mkdir /root/.ssh/
COPY deploy_key /root/.ssh/id_rsa
RUN chmod 600 /root/.ssh/id_rsa && \
ssh-keyscan github.com >> /root/.ssh/known_hosts && \
Copy link
Contributor

Choose a reason for hiding this comment

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

does this reduce the authenticity? is there a precedent for doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vishwa used this pattern in the pwasm-microservice

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