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/BE11 #215

Merged
merged 13 commits into from
Nov 29, 2022
Merged

Feature/BE11 #215

merged 13 commits into from
Nov 29, 2022

Conversation

BElifb
Copy link
Contributor

@BElifb BElifb commented Oct 29, 2022

  • DO NOT MERGE, I just needed to create pull request to test it.
  • Created the YAML file for CICD workflow.
  • The current workflow is supposed to be able to run the unit tests and build the DockerFile on every direct push or a pull request to master.
  • With the current state of the repo, it seems like a really good idea to have some sort of staging/testing branch before we merge things to main. Waiting for feedback on this from the reviewers as well.
  • You may also refer to issue BE-11: CI/CD Research and Implementation #209 for more detail.

@BElifb BElifb added Enhancement New feature or request Effort: Medium Priority: Medium This issue should be handled, if there isn't any high priority issue Idea labels Oct 29, 2022
@BElifb BElifb self-assigned this Oct 29, 2022
@KarahanS
Copy link
Contributor

KarahanS commented Oct 29, 2022

We should cd into App/backend folder to install the requirements. Also we should set the secret key and other information. Here it mentions about secret data. Also we have to give database names as well. It feels like we should dockerize the application in our CI/CD and run the unit tests in dockerized version or something in order to connect to the database. It feels like giving the db name won't work but not sure.

@KarahanS
Copy link
Contributor

Agreed, we should create another remote branch named develop and test CI/CD on that.

Added default working directory, hid the secret key.
@BElifb
Copy link
Contributor Author

BElifb commented Oct 29, 2022

We should cd into App/backend folder to install the requirements. Also we should set the secret key and other information. Here it mentions about secret data. Also we have to give database names as well. It feels like we should dockerize the application in our CI/CD and run the unit tests in dockerized version or something in order to connect to the database. It feels like giving the db name won't work but not sure.

  • I was aware of the directory problem, was searching for a way to define it for the entire workflow instead of seperate cd for each step. Updated default working directory accordingly.
  • I am just thinking outloud here, so correct me if I'm wrong. Aren't unit test supposed to be independent of the database.
  • Also I don't think we should have something named SECRET_KEY in the YAML file. Github has a secrets mechanism for that, but as far as I can see we need to be able to access settings in order to use it, so postphoned until next PS.
    • I also changed SECRET_KEY in the file, all we need to do is to add id in the PS with the same name.
    • We can do the same for other DB variables, added them in commented out version for now.

@KarahanS
Copy link
Contributor

KarahanS commented Oct 29, 2022

Running Django tests without db connection: https://stackoverflow.com/questions/5917587/django-unit-tests-without-a-db Maybe this could be helpful.

@BElifb
Copy link
Contributor Author

BElifb commented Oct 29, 2022

Running Django tests without db connection: https://stackoverflow.com/questions/5917587/django-unit-tests-without-a-db Maybe this could be helpful.

  • Looks like a good solution. Not examining in detail, since we're going to have to wait till next week to set up the secret variables.

@KarahanS KarahanS added the Status: On Hold More discussion is needed before handling this issue label Oct 30, 2022
Workflow for creating and publishing a Docker image to Github Packages
@BElifb
Copy link
Contributor Author

BElifb commented Nov 27, 2022

  • I took out testing job, since it is a task of its own to build a testing suit compatible to run on github actions. We can implement it as a separate workflow or add to this one later if need be.
  • Created a second workflow to build and publish image to Github Packages. (Create and publish a Docker image). This way you can download the image from anywhere and even use it as a base for creating other images.
  • You should be able to see the image in the main page of the repo, to the right, under Packages directory.
  • Before pulling the image you should create a personal access token and authenticate yourself in the terminal using it. Here's the link explaining the steps. For pulling the image read:packages persmission should be enough, but you may also add write:packages and delete:packages as well if you plan on pushing images in the future.
  • After creating the token, authenticate yourself in the terminal as in here.
  • Then pull the image using the command given in the image page (e.g. docker pull ghcr.io/bounswe/bounswe2022group8:pr-215)
  • Assuming you have docker-compose.yml and .production.env files available, you should be able to run a container created from the image via following command: docker-compose up -d

@BElifb BElifb added Status: In Progress The issue is being handled. and removed Status: On Hold More discussion is needed before handling this issue labels Nov 27, 2022
@BElifb
Copy link
Contributor Author

BElifb commented Nov 27, 2022

  • While testing pulling and running the image I received the following error:
    • failed to solve: rpc error: code = Unknown desc = failed to solve with frontend dockerfile.v0: failed to read dockerfile: open /var/lib/docker/tmp/buildkit-mount3131148562/Dockerfile: no such file or directory
    • Which basicly means that docker-compose couldn't find the Dockerfile. Which seemed odd at first since I already had an image built. But then I realized the line build: . in the docker-compose file was rebuilding the image. I am pretty sure this makes the initial build docker build --tag python-django . redundant.
    • Anyways, in order to use the existing image and not build a new one we need to replace the line build: . in docker-compose.yml with image: 6f6e28ad14dfdb7d13dbba74f597058f3b11e9014ed91c1ef3fbf20bd9d93114, where the very long string will be the name of the image in your docker desktop.
    • Right now we need to copy the local name from the docker desktop. But when I get access to repo settings I will enable public packages so that the image can be public. This will eliminate the need for the aforementioned personal acces token, and we will be able to pull the image directly inside docker-compose. Hence with automated build, we will be able to run the docker container with a single command docker-compose up -d.
    • I didn't make the change in the repo in case someone was using the old version. I am either going to change it or add a second docker-compose in a different location. Open to suggestions from reviewers.
    • I tested it and managed to run a container created from the pulled image, it would be better if at least one reviewer did the same thing and confirmed.

@BElifb
Copy link
Contributor Author

BElifb commented Nov 27, 2022

  • Another thing: if the frontend team can share the specifications for their dockerization, we can automate theirs as well. Either add to this workflow or create a separate one. @kostanya @sinemKocoglu

@BElifb BElifb added Status: Review Needed A review is needed for this issue and removed Status: In Progress The issue is being handled. labels Nov 27, 2022
@KarahanS
Copy link
Contributor

KarahanS commented Nov 27, 2022

  • I'm not sure if the deployment gets affected by the change in our docker image. You have to get in touch with @ooodogodogodogo for that.
  • For the frontend side, I rewrote the Dockerfile.production - however it's been a while since I used it therefore I'm not sure about the command. You build the image with something like this: docker build . -t frontend and then run it with docker run -p 80:80 -d frontend. Not really sure about the commands - I think, again the contact is @ooodogodogodogo at this point. (Btw, maybe we can find the related commands from Discord - we were writing them down while trying to deploy the product)

@BElifb
Copy link
Contributor Author

BElifb commented Nov 27, 2022

  • I'm not sure if the deployment gets affected by the change in our docker image. You have to get in touch with @ooodogodogodogo for that.
  • Well I was thinking we could use the image in the repo for deployment as well. What we're using is basicly a Docker Hub alternative.
  • For the frontend side, I rewrote the Dockerfile.production - however it's been a while since I used it therefore I'm not sure about the command. You build the image with something like this: docker build . -t frontend and then run it with docker run -p 80:80 -d frontend. Not really sure about the commands - I think, again the contact is @ooodogodogodogo at this point. (Btw, maybe we can find the related commands from Discord - we were writing them down while trying to deploy the product)
  • I am a little confused since I saw 2 Dockerfile s in frontend (one is Dockerfile.production). Also think we're not using docker-compose for this part?. I think there's a way to modifiy command for specifying ports etc., so that we can make it work.
  • Ball's in @ooodogodogodogo 's court at this point

@KarahanS
Copy link
Contributor

KarahanS commented Nov 27, 2022

  • Yep, for the frontend we have two different Docker files, one of which is for development - whereas the other is for deployment if I'm not mistaken. For the port stuff, as far as I remember we've bound backend to 3000 and frontend to 80.
  • Could you navigate @ooodogodogodogo actively at this point? Have some discord call, do it together or something? I fear this PR will be blocked for a long time if no one doesn't take the initiative for communicating. As far as I understand, these are the required actions:
    • Firstly, we'll try deploying our product again changing the dockerfile from the one present in the repo to the one in the Docker Hub. If it's working on deployment - then this functionality serves the purpose.
    • Then we'll update the dockercompose file so that it "pulls the image directly inside docker-compose.". This way, the need for a Dockerfile will be gone. Also in each merge, our dockerfile will get updated.
    • Then we'll try the same thing for frontend - which has some more question marks ahead. If it works, then we don't even use any dockercompose file or something (at least for the deployment). Because deployment side uses the image we provide - which will get updated each time we merge some PR to the repo.

@BElifb
Copy link
Contributor Author

BElifb commented Nov 27, 2022

  • I don't know if we're currently using docker hub for deployment, but if this works we won't need it at all. As I said this counts as a replacement.
  • To be clear we will still need the Dockerfile(s), but they will be used by the automated workflow, we just won't need to use them explicitly.
  • If you're refering to images having different tags, by updating dockerfile (meaning docker-compose I think) at each merge, we can configure it to have the same name hence override the previous image. But this will cause us to lose history of images. It's a choice we'll have to make.
  • Other than that I have done most of the work today, backend part is ready (needs a review), and frontend shouldn't take too long to add once I get the exact configuration/commands. + settings update with the assistant
  • I was thinking we could discuss deployment part with @ooodogodogodogo on Tuesday's meeting.
  • Btw review doesn't need to wait for settings update, can just follow the comments I wrote and use a personal access token.

Copy link
Contributor

@KarahanS KarahanS left a comment

Choose a reason for hiding this comment

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

Thanks for the efforts. I followed the necessary steps and checked if it's working properly. You are absolutely right on the issue that build: . already builds the Dockerfile, so we don't need to call that action beforehand. I added new command to the docker-compose file as a comment to let others know how to use it.
Also, I had some problems while making the configurations from terminal. So let me provide the necessary commands directly (so that everyone can follow):
After creating a token, open a terminal and write the following.

docker login ghcr.io --username <YOUR-USERNAME>

Your username stands for the username you are using for Github. For example, mine is KarahanS. Upon executing this command, it will prompt for a password - copy paste your token. Now, you are done with the authentication. As you stated above, one should execute the following line:

docker pull ghcr.io/bounswe/bounswe2022group8:pr-215

Everything looks good to me, you can merge the PR.

@KarahanS
Copy link
Contributor

KarahanS commented Nov 29, 2022

Hi @suzan-uskudarli @busraoguzoglu, we are trying to add a new feature that will enable everyone (at least from our team) to pull the necessary Docker image for backend from Github Packages (as if it's a public image out there) and compose it with the docker-compose file in milliseconds without needing to build it again as it will be done automatically in CI/CD.
However, to be able to pull it from public, we have to update the package settings. We can't do it since it requires authorization.
Upon navigating here, you should be able to see Package Settings under Open an Issue. It's something visible to you - not to us so I can't put a screenshot regarding to that. We thought maybe you can set the package at least public to bounswe so that everyone in the team can use it like a public image (without all those configurations with Github access tokens, pulling image and giving its ID to the docker-compose file and so on). We tried to update it with @busraoguzoglu in the lab, however she didn't have the permission to change that. Waiting for your actions @suzan-uskudarli at this point. Thanks

@KarahanS KarahanS added Approved This work is reviewed and approved by a team member and removed Status: Review Needed A review is needed for this issue labels Nov 29, 2022
@mumcusena mumcusena merged commit 39173a8 into master Nov 29, 2022
@KarahanS KarahanS deleted the feature/BE-11 branch December 2, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved This work is reviewed and approved by a team member Effort: Medium Enhancement New feature or request Idea Priority: Medium This issue should be handled, if there isn't any high priority issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants