Skip to content

Comments

Qwerty1q2w dev#25

Closed
qwerty1q2w wants to merge 7 commits intoeramba:1.xfrom
qwerty1q2w:qwerty1q2w-dev
Closed

Qwerty1q2w dev#25
qwerty1q2w wants to merge 7 commits intoeramba:1.xfrom
qwerty1q2w:qwerty1q2w-dev

Conversation

@qwerty1q2w
Copy link

@qwerty1q2w qwerty1q2w commented Oct 26, 2022

Hello! I suggest to use variables in code and configs.

But anyway I see errors with c hard-coded values

At least in cron container
/var/www/eramba/app/upgrade/logs/cli-error.log:#0 /var/www/eramba/app/upgrade/vendor/robmorgan/phinx/src/Phinx/Db/Adapter/PdoAdapter.php(84): PDO->__construct('mysql:host=mysq...', 'docker', 'docker', Array)

@qwerty1q2w
Copy link
Author

qwerty1q2w commented Oct 26, 2022

Also I suggest not calling envs in cron jobs. We call envs from .main.env before and I suggest .env now.

@shrkz1 shrkz1 assigned shrkz1 and unassigned shrkz1 Oct 26, 2022
@qwerty1q2w
Copy link
Author

qwerty1q2w commented Oct 26, 2022

You may change passwords in .env to "docker". At least for working now.

@qwerty1q2w
Copy link
Author

qwerty1q2w commented Oct 26, 2022

Changes in crontab/crontab probably you may change in docker image too and remove string in docker-compose file in volumes section.

@shrkz1
Copy link
Member

shrkz1 commented Oct 26, 2022

I like the idea of moving those environmental values like that within the .env file, but your solution overall has some issues. In order to consider merging it, there are few changes you have to make. I made a branch out of your branch with changes that makes it work - 1.x...qwerty1q2w-dev

  1. Needed to remove the $ sign because my shell was evaluating it as variable and it didnt work properly b7b4e73

image

  1. When doing a docker compose with environmental values defined within the YML file, these environmental values that are within CRON container wont get propagated into the cron job. So if you tried for example to print out all environemntal values during a minute cron job like:
* * * * printenv > /tmp/cron-envs.txt

it wont contain the environemntals started with the CRON container.
The only work around i know is to have those environmental values that were defined with start of the CRON container, put into some text file and let them get loaded into the cron job. So my solution to your changes would be:
Making an entrypoint for CRON container that saves ENV values into a file:
1f3bc4e

and running cron jobs using that file:
0158128

If you merge my changes into your PR branch (or do it somewhat alike), we can test it porperly here and possibly put it into 1.x release.

@qwerty1q2w
Copy link
Author

My attempts.
image
I will try to call printenv in cron

Errors in .env with $ is normal behavior. You can use a $$ (double-dollar sign) when your configuration needs a literal dollar sign.

@qwerty1q2w
Copy link
Author

@shrkz1 New PR with your changes. #26

@qwerty1q2w qwerty1q2w mentioned this pull request Oct 26, 2022
@shrkz1
Copy link
Member

shrkz1 commented Oct 27, 2022

Hello, i wanted to point out that modifying crontab/crontab file does not automatically reflect changes in the application - look at screenshot below:
image
crontab -u www-data -l tells the actual state of crontab for www-data user

thats why in the other pull request, i had to make an entrypoint for cron container where i "initialize" the updated crontab file: 1f3bc4e#diff-aecca9bd4875b79b177e8af0a3e93388c3b188ec97daeebce66f42126ae4f52bR9

lets continue with the other PR (#26)

@shrkz1
Copy link
Member

shrkz1 commented Oct 31, 2022

Im closing this in favour of #26

@shrkz1 shrkz1 closed this Oct 31, 2022
@shrkz1 shrkz1 added the duplicate This issue or pull request already exists label Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants