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

Feat: added env support to Dokploy stack compose #637

Open
wants to merge 10 commits into
base: canary
Choose a base branch
from

Conversation

xenonwellz
Copy link

No description provided.

Copy link
Contributor

@Siumauricio Siumauricio left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR!

This works for stack, but is breaking the compose, I researched a little bit more about this problem and there is a option called docker stack config https://docs.docker.com/reference/cli/docker/stack/config/ and this command takes the provided stack file and it perform any neccesary mergind and variable sustitution(such enviroment variables) and outputs the result

I think is better this approach since we don't need to apply any regex validation to replace the values

Screenshot 2024-11-01 at 12 00 53 PM

Screenshot 2024-11-01 at 12 01 38 PM
Screenshot 2024-11-01 at 12 03 00 PM

@xenonwellz
Copy link
Author

I thought of this too and initially looked to do this if it was possible.

The issue with it was, we'd have to export the env variables to the current shell which will not work with the spawnAsync function, but could be solved by parsing the .env file and passing it as the SpawnOption.env argument, then pass a command to generate the config file, this also means we will need to call another spawnAsync function. and for some reasons, it keeps throwing ENOENT error.

Also, I felt it would make more sense to structure the functions to match what we had before, writing files directly with writeAsync, and I decided to use regex to avoid installing a template rendering package.

I will look at the bug that breaks the docker-compose, and will push a fix.

Thanks.

@xenonwellz
Copy link
Author

Apparently, the issue was, I did not generate a processed file when building without stack so it fails with an error indicating it could not find docker-config.processed.yml

@xenonwellz
Copy link
Author

I have also added stop functionality for stack.

@Siumauricio
Copy link
Contributor

Thanks, I will take a closer look at the docker stack config, because with this solution it does not work using this syntax

env_file:
  - .env

However using docker stack config it resolve the variables correctly

@xenonwellz
Copy link
Author

Okay, I will try the docker stack config once more.

@xenonwellz
Copy link
Author

I have updated it to use docker config. I was able to find my way around the bug i had earlier.

Copy link
Contributor

@Siumauricio Siumauricio left a comment

Choose a reason for hiding this comment

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

It works great, thanks, I left you a few extra comments, let me know what you think!

Copy link
Contributor

@Siumauricio Siumauricio left a comment

Choose a reason for hiding this comment

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

Check the request

@xenonwellz
Copy link
Author

Log removed as requested

Copy link
Contributor

@Siumauricio Siumauricio left a comment

Choose a reason for hiding this comment

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

I have tried with source type from github and it doesn't seem to work, neither with multi server feature it breaks it doesn't run docker compose correctly,

it only works when you use raw, I think this needs to be tested more, as we are definitely forgetting certain cases and this makes them break, and I'm afraid of that.

I think this might take a little longer, would you mind creating tests that are in the test folder, as I see that many cases fail and to prevent that it would be better to do that.

@Siumauricio Siumauricio added the enhancement New feature or request label Nov 7, 2024
@xenonwellz
Copy link
Author

xenonwellz commented Nov 8, 2024

If i could see the logs, that will be very helpful. I have been able to identify the issues with non-raw file types, I will also look to the bug issues with remote server.

And about writing test for this, I have not worked with vitest

@xenonwellz
Copy link
Author

I have tested for multi server feature and other source types, And confirm they work.

Test again from your end to confirm

@Siumauricio
Copy link
Contributor

Using this example is not working

Looks like the processed file is being created outside of the path /stack
Screenshot 2024-11-10 at 7 01 08 PM
Screenshot 2024-11-10 at 7 01 56 PM

@xenonwellz
Copy link
Author

xenonwellz commented Nov 11, 2024

Let me see the error in your console and the deployment log, Cos it worked for me and it is meant to create a processed file in he root directory so that is actually correct

EDIT:
The issue occurs because the processed file is not in the same directory as the env file.
I will put the processed file in the compose directory. Exactly how the env file is being generated.

@Siumauricio
Copy link
Contributor

Thanks for the changes, I will check it this weekend, I have to test it well to cover any case, sorry for the delay.

@xenonwellz
Copy link
Author

Still waiting for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants