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

chore: Added a Dockerfile to be able to easily build and test. #615

Closed
wants to merge 1 commit into from
Closed

chore: Added a Dockerfile to be able to easily build and test. #615

wants to merge 1 commit into from

Conversation

marcusber
Copy link
Contributor

Fixes

Adds a Dockerfile for building and testing the solution. Makes the solution more accessible for contributing developers.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
    • Tests passes using the instruction in the readme.
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified
    • No code modifled.

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

Can this be merged in with the existing Dockerfile?

@marcusber
Copy link
Contributor Author

@childish-sambino I didn't really understand the usage of the existing one, so I created a new.

docker build -t twiliotest .
docker run --rm -it twiliotest /bin/bash

dotnet build does not work and make does not exist.

If the purpose is to be able to build and test the solution they could of course be merged.

@marcusber
Copy link
Contributor Author

The documentation stated:

The Dockerfile present in this repository and its respective twilio/twilio-csharp Docker image are currently used by Twilio for testing purposes only.

So if it also has other purposes, the change should perhaps be done by https://github.com/twilio/ in order to make sure that it is still valid for testing purposes.

@JenniferMah
Copy link
Contributor

Hi @marcusber,
Could you clarify you mean when you said dotnet build does not work and make does not exist. Are you not able to run tests locally with the make test command?

@marcusber
Copy link
Contributor Author

@JenniferMah Of course.
In the existing Dockerfile in the repo with the commands I provided above it did not build (several errors and tons of warnings) and make was not installed on that image.

When it comes to my local computer I do not have all those legacy frameworks installed, so using Docker was easier for me. So, no, it did not build on my local installation of VS2022 (net6 and some more, but not enough to build the repo). Also, I don't have make on my windows machine.

The Dockerfile in this PR installs all that. Building as well as all tests works (both using dotnet build and make). Using the commands in the README.md you could build and update the code in the repo, without being forced to rebuild the docker-image after every update.
It still gives up a ton of warnings, mostly due to malformed documentation.

Sorry about the confusion.

@childish-sambino
Copy link
Contributor

I'm in favor of merging the changes into the existing Dockerfile. I can test afterwards in our internal process to make sure everything still works properly.

One thing we do need for the internal process is the steps:

COPY src ./src
COPY test ./test
COPY Twilio.sln .

We want to ensure we have the source in the image (instead of coming from a mounted volume).

This should still work with your documentation additions as you can still mount the volume for more rapid local development; it will just either reside alongside or replace the /twilio contents in the running container.

Let me know if more clarification is needed.

@marcusber
Copy link
Contributor Author

Closed in favor of #623

@marcusber marcusber closed this Jul 21, 2022
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