search

petio-team / petio

Petio Request, Discover, Review
https://petio.tv
MIT License
249 stars 28 forks source link

Correct object by removing dataValue #823

Closed HawkinsR closed 2 months ago

HawkinsR commented 1 year ago

Master/Main branch stability correction. Currently, Main does not build in Docker. This change should correct build errors, and allow for a stable (if outdated) functionality.

ADRFranklin commented 1 year ago

This has already been fixed on the dev branch quite some time ago.

HawkinsR commented 1 year ago

I'm glad to hear it!

It's such an uncommon issue to have a broken main branch that there's not really a good way to draw attention to it except with a PR like this.

If anyone tried to pull and build main, it would fail. Anytime a user runs the docker-compose, they're pulling an outdated image that is behind main from the last time a successful image was pushed to the container repo. That is, unless the dev branch is being deployed to the container repo as the production image.

ADRFranklin commented 1 year ago

The docker image on the main branch builds fine for me, I've have built it many times over the last few months. But the last changes to master were quite some time ago, so the latest image should work perfectly fine and there shouldn't be a need to build it manually.

I'm not sure why you think the main image is outdated though?

HawkinsR commented 1 year ago

Perfectly valid. You're right, I don't know for sure. But looking at the repo, it's my best guess at what would have happened. The repo has no automation on master to prevent breaking changes getting pushed or merged or to flag a bad build, so it would be entirely up to the person running the build and deployment manually to catch. After a long day of merging, bug-fixes, and minor patches, nobody is in a mood to think about the image deployment. And to make it worse, it's subtle. You'll still get a successful deployment, tagged with latest, but it's not the correct version, and won't match the code in the repo.

I'd be happy to screen cap and share exactly what I did. The source code would not build. I cloned the repo, and from the master branch tried to run docker build .. It failed. Not just the image build failed, but the npm run build step associated with the admin service failed. I examined the file, deleted a few duplicate lines, tried a docker build . again, and it built. I ran the compose (using the local image I'd just built, and the remote MongoDB image), and everything spun up OK.

Clone the master branch and try a docker build, not a compose up. It'll fail the build.

Docker is renowned for it's tendency to cache. If you build successfully, it will hold that image locally. Especially if you've just tagged it as latest, you can still think of it as v1. If you then modify the source with breaking code (v2), and try to build again, it doesn't clear the old image and overwrite with a broken v2, it fails to build, and the local image retains it's latest tag and keeps running the v1 code. And if you push the image tagged latest it'll push to the container repo, but it's still v1.

When you run docker-compose and tell it to look for the image, it will check locally, then look remotely. If there is a working version of the image on your local machine, it will prefer that over looking for a new version in the repo, unless specified in the compose.

The startup docs instruct on how to spin up with docker-compose, so anyone doing so will pull the compose, then try to spin up the containers. If they haven't cloned and built (as I did) then docker will try to pull the image from your container repo. Likely the v1 version of latest if there was no correction to the code after the failed build.

ADRFranklin commented 1 year ago

That is because we don't build and push to master at all, everything goes through dev branch, which when that is at a point where everything is stable, we merge into the preview branch so people can test the stable build, and any issues will be addressed and after all issues are fixed, we then merge the dev branch into master. We won't be able to merge into master if the dev build fails at any point in CI.

As for building with docker the latest tag is not the tag I use when testing the docker image, I use the current git sha1 commit to tag the image, while also tagging the same image as latest if needed. However I am also very aware of docker caching however you can also tell docker to build without using the cache at all, which is something I do when pushing final releases.

I am not sure why you are not able to build. I build and work on Petio in a devcontainer, which means that I don't have any interference while working on it, no system changes, no caching issues (that I don't already know about), this provides me with the best environment to work with. Though even in this completely fresh and clean environment I am able to pull and build the image just fine, and run it without any problems.

Also the repo does have automation on master, it's all done via CI aka GitHub workflows/actions. Everything is automated as much as we can make it.

I have used and worked with docker both using windows/linux containers for quite some years now, making sure every project I used practically used it, I have plenty of hours spent optimising images, and coming to understand the build cache and how each docker update changes the system, so I am aware of it, and know how to fully make use of it.

PS: I am going to be moving the build system to Earthly in the future, as I do love eco system for building images.

HawkinsR commented 1 year ago

@ADRFranklin, I apologize if I offended in any way. I in no way meant to imply you or the rest of the Petio team don't understand the technologies you're using, and I'll be the first to admit that you'll understand your systems better than I do.

I only submitted the PR as as way to alert your team to a break in your main branch. I understand the workflow you're following with regards to your branching strategy. But if the problem outlined in this PR was fixed in your dev branch, then it hasn't been merged to main, and the break remains.

Again, I will immediately admit that I am taking my best guess as to the image currently available. But if you (or your automation) tag by latest, and your compose is pulling the image tagged latest, the problem remains that the last build done from main would have failed, and the version being hosted not fully updated.

With all due respect, try it. Try it clean. Spin up a sterile environment, clone main, and try the build. With only the source to work with, you can't even build the service manually until you edit the code as the PR details. If the code doesn't build, then I don't care what container you have published, if it's running, it's not the code you have in main.

ADRFranklin commented 1 year ago

@HawkinsR I in no way took anything you said as an insult, so no need to apologise, I am just dumb founded in how you managed to find a problem building it, which I can't seem to replicate.

I've built the image 4 times today alone and haven't ran into any fails. So I am more curious and or interested in the cause of it. There is a chance that there could be something I missed that is preventing me from getting this error, but I can't imagine that is the case. I just created a brand new Ubuntu vm, install docker, pulled project and built it again and got no errors. I am not entirely sure what could possibly be going wrong here for you.

HawkinsR commented 1 year ago

Fair enough. It's hard to distinguish frustration from insult over comment boards.

Is there a better communication channel we can try to work through to replicate the issue? I have a log of the build output I can share with you, as well as the relevent specs for my ENV if you think that might be a factor.

ADRFranklin commented 1 year ago

@HawkinsR feel free to join our discord server: https://discord.gg/bseGmrUd3N we can talk more there if you feel it is suitable for you.