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

Added automatic docker build on merge to master #323

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andypenno
Copy link

@andypenno andypenno commented Aug 21, 2024

Hey, loving the project!

I tried to get this working on podman, but some issues with second order dependencies were causing the build to fail. After a couple of days of playing whack-a-mole I figured it would make more sense to add support for building using a GitHub actions workflow. I noticed issues with a similar PR due to the frontend needing a way to provide the backend API url to the client, so I've created a simple helper lib function within the frontend to provide the necessary definitions.

There's container images available to test on my dockerhub profile, although I've ensured all changes won't affect the local build process if people prefer to use that method instead.

Let me know what you think, I'm happy to make further changes if there's any concerns!


This PR includes:

  • Automatic build & push to docker hub when pushing to master, or when triggering a workflow dispatch on master
  • Automatic docker build on pull requests to validate changes
  • Updated the docker-compose.yaml to use the container images, rather than building locally
  • Added support for defining backend settings using the container environment
    -> All options defined in config.toml can now be loaded instead from the environment
    -> Order of precedence is environment definition, config.toml, and finally default inline configuration defined in config.ts
  • Added support for defining frontend settings using the container environment
    -> Added a library function to fetch the server environment definitions
    -> Modified existing calls to process.env to use the new library function
    -> Left the initial statically compiled environment definitions in place as a backup definition, if no environment definitions are provided

Remaining tasks to complete before able to merge to ItzCrazyKns/Perplexica:

  • Add secret definitions for DOCKER_USERNAME and DOCKER_PASSWORD to ItzCrazyKns/Perplexica to ensure push to dockerhub works on base branch

This PR includes:
- Automatic build & push to docker hub when pushing to master, or when triggering a workflow dispatch on master
- Automatic docker build on pull requests to validate changes
- Updated the docker-compose.yaml to use the container images, rather than building locally
- Added support for defining backend settings using the container environment 
  -> All options defined in config.toml can now be loaded instead from the environment
  -> Order of precedence is environment definition, config.toml, and finally default inline configuration defined in config.ts
- Added support for defining frontend settings using the container environment
  -> Added a dynamic api route to load the container environment definitions on the server, and provide them to the client
  -> Added a library function to fetch from the newly created API, caching the response in the session storage
  -> Modified existing calls to `process.env` to use the new library function
  -> Left the initial statically compiled environment definitions in place as a backup definition, if no environment definitions are provided

Remaining tasks todo before able to merge to [ItzCrazyKns/Perplexica](https://github.com/ItzCrazyKns/Perplexica):
- Add secret definitions for `DOCKER_USERNAME` and `DOCKER_PASSWORD` to [ItzCrazyKns/Perplexica](https://github.com/ItzCrazyKns/Perplexica) to ensure push to dockerhub works on base branch
- Update documentation with information about changes
@ItzCrazyKns
Copy link
Owner

There isn't a need for the environment variable route since the getServerEnv function can be marked as a server function (by putting "'use server'" on top of the lib file) and the environment variables will be exposed to it by default.

…umentation to list environment variables (#9)

- Use server side rendering in the `getServerEnv` library function, rather than using the API route
- Updated `README.md` to list the relevant environment variables for the frontend and backend
- Updated `NETWORKING.md` to reflect the changes made in `docker-compose.yaml`
@andypenno
Copy link
Author

Good to know, I'm still getting used to NextJS. Latest commit refactors to remove the API route, and use server-side rendering on the getServerEnv library function. I've used a switch statement to maintain the fallback to the statically compiled environment definitions provided in the container build. I've also included some minor updates to the documentation:

  • Updated README.md to list the environment variables that have been added
  • Updated NETWORKING.md to reflect the changes I've made to the docker-compose.yaml

@ItzCrazyKns
Copy link
Owner

And the rest of the changes LGTM

@andypenno
Copy link
Author

What will you be using for the value of DOCKER_USERNAME? I'm assuming 'itzcrazykns', but just want to confirm.

@ItzCrazyKns
Copy link
Owner

What will you be using for the value of DOCKER_USERNAME? I'm assuming 'itzcrazykns', but just want to confirm.

Its itzcrazykns1337, I will first push the images to hub before merging this PR

@andypenno
Copy link
Author

Nice one, I've updated the appropriate references. Once you've added the secrets we should be good to go 👍

@ItzCrazyKns
Copy link
Owner

The changes to the reviews are still pending.

@andypenno
Copy link
Author

I'm not seeing any reviews on my end, was there another change that was requested that I'm not seeing?

@ItzCrazyKns
Copy link
Owner

I'm not seeing any reviews on my end, was there another change that was requested that I'm not seeing?

There are a few pending reviews in the changes.

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.

2 participants