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

Issue #33 docker image fix #34

Merged
merged 3 commits into from
Mar 21, 2021

Conversation

jn9e9
Copy link
Collaborator

@jn9e9 jn9e9 commented Mar 17, 2021

change e2e tests to use parallaxsecond github container registry docker image

Fix for #33

Signed-off-by: John 9e9 [email protected]

@ionut-arm
Copy link
Member

We could integrate the changes from this repo in the main image and use that one on the CI here as well without any docker build. Or we could publish it separately if we don't want to "pollute" the main image with client-specific stuff

@hug-dev
Copy link
Member

hug-dev commented Mar 18, 2021

We could integrate the changes from this repo in the main image and use that one on the CI here as well without any docker build

I am in favour of doing that! That way we keep the philosophy of one image fits all. Shouldn't increase the size that much, I hope, as it's just the Go compiler?

@hug-dev
Copy link
Member

hug-dev commented Mar 18, 2021

Also I am thinking that for a Getting started guide, we could include examples of Go and Rust clients there, that people can browse and execute.

@@ -99,8 +99,8 @@ if [ "$PROVIDER_NAME" = "tpm" ] || [ "$PROVIDER_NAME" = "all" ]; then
tpm_server &
TPM_SRV_PID=$!
sleep 5
tpm2_startup -c -T mssim
tpm2_changeauth -c owner tpm_pass
tpm2_startup -c 2>/dev/null
Copy link
Member

Choose a reason for hiding this comment

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

If it's that script that the container is meant to run, you could also add the Go installation steps here? So at least you wouldn't need to build the container.
Not ideal though if you want to run the script locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

script gets used locally and in CI - see comment: #34 (comment)

@jn9e9
Copy link
Collaborator Author

jn9e9 commented Mar 18, 2021

having the go testing image in the go client repo means that the client is in charge of which go versions it uses (and could test multiple go versions, if needed). Doing the go install in the docker container, rather than ci.sh provides a balance for the e2e test - go version doesn't change much, so don't want to have to re-install every time you run e2e test - dependencies, and go build, etc do want doing each time, to make sure you're testing the go code thats in your git repo. Publishing the go extended docker images would make sense - probably needs doing under another PR, as currently not sure how to do it and will require coordination with somebody with permissions on the repo to push to container repo, i guess?

@jn9e9
Copy link
Collaborator Author

jn9e9 commented Mar 18, 2021

We could integrate the changes from this repo in the main image and use that one on the CI here as well without any docker build. Or we could publish it separately if we don't want to "pollute" the main image with client-specific stuff

the non-pollution is a good thing to maintain IMO. If we could push a go image, that would be best I think, as docker would just add the go specific layers to the end, so be reasonble experience for users (not much extra image download)

@hug-dev
Copy link
Member

hug-dev commented Mar 18, 2021

the non-pollution is a good thing to maintain IMO

Yes you might be right and when thinking about it, the image we store currently in the GitHub registry is really the Parsec image, as in the image you need to start Parsec with all providers. Not so much a image to use by clients.

I am wondering if it would be possible to start Parsec inside the container, in isolation, and then have the Go client outside of it, either directly on the bare machine or in its own container. I think it's possible by mounting the socket path to both worlds.

@jn9e9
Copy link
Collaborator Author

jn9e9 commented Mar 18, 2021

the non-pollution is a good thing to maintain IMO

Yes you might be right and when thinking about it, the image we store currently in the GitHub registry is really the Parsec image, as in the image you need to start Parsec with all providers. Not so much a image to use by clients.

I am wondering if it would be possible to start Parsec inside the container, in isolation, and then have the Go client outside of it, either directly on the bare machine or in its own container. I think it's possible by mounting the socket path to both worlds.

It should be possible, yes - just mount as volume with paths in both filesystems - may investigate for testing whilst developing, but this works for now and provides reproducable environment for the testing.

Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

It's good for now! Let's see in the future if we want to publish a new Go client image or have the Parsec service running inside its container.

@hug-dev hug-dev requested a review from ionut-arm March 19, 2021 10:09
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

My only remaining question is - is there a need for a Dockerfile per provider configuration now that the image is common? Do you expect anything else to change between them (apart from maybe some parameters to the script running inside)?

@jn9e9
Copy link
Collaborator Author

jn9e9 commented Mar 19, 2021

My only remaining question is - is there a need for a Dockerfile per provider configuration now that the image is common? Do you expect anything else to change between them (apart from maybe some parameters to the script running inside)?

so they would take different features for the parsec build, but that could be passed through as a parameter to the docker file. I think we'd need to create a different tagged image for each variant to avoid the re-building.

I've added a note to that effect to #17

Failing that, i could just delete the non 'all' ones until we get round to looking at #17 ?

@ionut-arm
Copy link
Member

The combination of container image and ci.sh script can run any provider (including all-providers), so starting from that you can run the tests against any backend.

You might need to strap other things on top of the image (e.g. versions of Go), but I wouldn't think you need an image per provider since the client setup should be identical, right?

@jn9e9
Copy link
Collaborator Author

jn9e9 commented Mar 19, 2021

The combination of container image and ci.sh script can run any provider (including all-providers), so starting from that you can run the tests against any backend.

You might need to strap other things on top of the image (e.g. versions of Go), but I wouldn't think you need an image per provider since the client setup should be identical, right?

have now removed the unused e2e tests. will consider re-instating (and approach with the new docker file) in #17. likely will leave as is with only all.

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Thank you! 🙏🏻

@ionut-arm
Copy link
Member

Oh, it seems there's a conflict with the main branch

@ionut-arm ionut-arm merged commit e5d70ac into parallaxsecond:master Mar 21, 2021
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