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

refactor: build only one frappe/erpnext image #1032

Merged
merged 18 commits into from
Jan 15, 2023

Conversation

revant
Copy link
Collaborator

@revant revant commented Dec 28, 2022

No description provided.

revant added a commit to revant/bench that referenced this pull request Dec 28, 2022
@revant
Copy link
Collaborator Author

revant commented Dec 28, 2022

@sdfateh

check this migration guide https://github.com/revant/frappe_docker/blob/refactor-single-image/docs/migrate-from-multi-image-setup.md

Anyone can test this using frappe/erpnext:v14.11.1 image.

@revant
Copy link
Collaborator Author

revant commented Dec 28, 2022

I'll merge by next week. It'll be easier for me to maintain it.

@revant
Copy link
Collaborator Author

revant commented Dec 31, 2022

https://github.com/revant/frappe_docker/blob/refactor-single-image/docs/custom-apps.md#load-custom-apps-through-json

should the Apps json be base64encoded string?
easy to move around without causing character escape issues.
json looks simple but simplicity can be achieved by piping json to base64 and passing that to build-arg.

anyone?

@WebsiteDeveloper
Copy link
Contributor

https://github.com/revant/frappe_docker/blob/refactor-single-image/docs/custom-apps.md#load-custom-apps-through-json

should the Apps json be base64encoded string? easy to move around without causing character escape issues. json looks simple but simplicity can be achieved by piping json to base64 and passing that to build-arg.

anyone?

I would say it should probably be a json file for readability and not be base64 encoded

@WebsiteDeveloper
Copy link
Contributor

I'll merge by next week. It'll be easier for me to maintain it.

Would there be a way to maintain both?

@revant
Copy link
Collaborator Author

revant commented Dec 31, 2022

not really.

@revant
Copy link
Collaborator Author

revant commented Jan 1, 2023

I made it BASE64 string. It was failing for cases where git token/password had # in it. Need something that is environment variable friendly

It is used like this:

mkdir /opt/frappe && echo "${APPS_JSON_BASE64}" | base64 -d > /opt/frappe/apps.json;

For readability

export APPS_JSON='[
  {
    "url": "https://github.com/frappe/payments",
    "branch": "develop"
  },
  {
    "url": "https://github.com/frappe/erpnext",
    "branch": "version-14"
  },
  {
    "url": "https://user:password@git.example.com/project/repository.git",
    "branch": "main"
  }
]'
export APPS_JSON_BASE64=$(echo ${APPS_JSON} | base64 --wrap=0)

Now you can even create a file /path/to/apps.json and feed it like export APPS_JSON_BASE64=$(cat /path/to/apps.json | base64 --wrap=0)

@revant
Copy link
Collaborator Author

revant commented Jan 1, 2023

It will be easy to build version-12, version-13 and version-14 apps. I tried building all version successfully.
anyone who is facing any asset builder issue or asset population issue this will solve it.

Frontend container has contained assets for it to serve. Backend and workers have the assets in case they require them, even socketio will have them if needed in future or in custom apps. There is no need for re-population of assets because the container gets its own assets.

@sdfateh @lekhnath do try with your version 13 custom apps.

@sdfateh
Copy link
Contributor

sdfateh commented Jan 1, 2023

@revant

I think we should give that more time, in order to do experiments and approval. A slight effect on production could eliminate the idea of ​​using a container.

I really appreciate your effort, but please keep it slow.

@revant
Copy link
Collaborator Author

revant commented Jan 1, 2023

how much more time is enough?

@sdfateh
Copy link
Contributor

sdfateh commented Jan 1, 2023

@revant
I think it's not about time, it's about testing and verifying.

@sdfateh
Copy link
Contributor

sdfateh commented Jan 1, 2023

I have this error with frontend now :

Error response from daemon: Cannot restart container 91cf090169f2404a9d69d7d6a5b7aab6fb5aab86c1e21ed23ec1da1c314ab08c: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nginx-entrypoint.sh": executable file not found in $PATH: unknown

Any advice ?

@revant
Copy link
Collaborator Author

revant commented Jan 1, 2023

  • is nginx-template.sh it in path? exec into container and find out. docker run --rm -it frappe/erpnext:v14.11.1 which nginx-entrypoint.sh
  • It needs to get copied, it needs to be executable, it needs to be in path.

Confirm build arch as well, sometimes scripts don't execute if image is pulled for different arch.

@sdfateh
Copy link
Contributor

sdfateh commented Jan 2, 2023

@revant

I added :

RUN chmod 755 /usr/local/bin/nginx-entrypoint.sh

to my dockerfile to have the x permission to scripts ,
I see that your image script has x permissions to the scripts but in the dockerfile no chmod

any advice?

@revant
Copy link
Collaborator Author

revant commented Jan 2, 2023

any advice?

All the devices I use are linux based. May be it preserves the permission for me on git clones. Also in cluster/kaniko executor it pulls as linux and preserves permission.

Is there a way you can chmod +x nginx-entrypoint.sh on your local machine and then build?

@sdfateh
Copy link
Contributor

sdfateh commented Jan 3, 2023

I solved it for me by adding RUN chmod 755 /usr/local/bin/nginx-entrypoint.sh to the Dockerfile,
So I just ask you for your opinion, If we have to add it to the dockerfile let us edit the PR and add it.

@revant

@revant
Copy link
Collaborator Author

revant commented Jan 3, 2023

So I just ask you for your opinion, If we have to add it to the dockerfile let us edit the PR and add it.

As you're telling me to add, I'll add a RUN layer with chmod. only that? anything else?

you can add review on GitHub? that way i can resolve one by one. you're the only expert who is commenting, may be after you add review others will also add their review.

@sdfateh
Copy link
Contributor

sdfateh commented Jan 4, 2023

@revant What do you mean by review on GitHub, A new issue on this repo, or on the PR?

@sdfateh
Copy link
Contributor

sdfateh commented Jan 4, 2023

I have another problem, after using the new one image, something important changed in the MariaDB,
When creating sites on the old way, that is user list:

MariaDB [(none)]> select host, user from mysql.user;
+-----------+-------------+
| Host      | User        |
+-----------+-------------+
| %         | devops1     |
| %         | root        |
| localhost | mariadb.sys |
| localhost | root        |
+-----------+-------------+
4 rows in set (0.002 sec)

But in the new it is like this:

MariaDB [(none)]> select host, user from mysql.user;
+--------------+-------------+
| Host         | User        |
+--------------+-------------+
| %            | root        |
| 10.10.104.10 | devops1     |
| localhost    | mariadb.sys |
| localhost    | root        |
+--------------+-------------+
4 rows in set (0.004 sec)

This means If the container changed its IP, the site will be Internal Server Error.
Note: If sites exist the migration from old to new works well.

Have any idea to work with this ?

@revant
Copy link
Collaborator Author

revant commented Jan 4, 2023

there's is no patched bench any more.

official bench from pip install is used. so you need --no-mariadb-socket while creating new site

mentioned here https://github.com/revant/frappe_docker/blob/refactor-single-image/docs/migrate-from-multi-image-setup.md#configurator

@sdfateh
Copy link
Contributor

sdfateh commented Jan 5, 2023

@sdfateh

check this migration guide https://github.com/revant/frappe_docker/blob/refactor-single-image/docs/migrate-from-multi-image-setup.md

I test the new image and build my 4 custom apps. and I have approval from the testers now. also I tried v13 & v14 and it looks good.
Also, the migration for existing data is OK.

I think we can go ahead with the new single image ! 🥳
Let's say after 1 week. is that good or you?

@revant
Copy link
Collaborator Author

revant commented Jan 5, 2023

Sure we'll merge next week? Monday 16th

@revant
Copy link
Collaborator Author

revant commented Jan 7, 2023

PWD stopped working somehow. On PWD instance it only starts db and redis. Locally it starts all services. Following seems to be working locally.

docker swarm init
wget https://raw.githubusercontent.com/revant/frappe_docker/refactor-single-image/pwd.yml
docker stack deploy -c pwd.yml pwd

Edit:

fixed by adding volumes for logs and assets.

@africlouds
Copy link
Contributor

africlouds commented Jan 15, 2023

EDIT: my bad, it was a typo in apps.json

1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
[4/5] Linking dependencies...
warning " > @frappe/esbuild-plugin-postcss2@0.1.3" has unmet peer dependency "less@^4.x".
warning " > @frappe/esbuild-plugin-postcss2@0.1.3" has unmet peer dependency "stylus@^0.x".
warning Workspaces can only be enabled in private projects.
[5/5] Building fresh packages...
Done in 36.19s.
Found existing apps updating states...
WARN: restart failed: Couldn't find supervisorctl in PATH
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/bench/commands/make.py", line 68, in init
    init(
  File "/usr/local/lib/python3.10/site-packages/bench/utils/render.py", line 105, in wrapper_fn
    return fn(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/bench/utils/system.py", line 90, in init
    install_apps_from_path(apps_path, bench_path=path)
  File "/usr/local/lib/python3.10/site-packages/bench/app.py", line 689, in install_apps_from_path
    apps = get_apps_json(path)
  File "/usr/local/lib/python3.10/site-packages/bench/app.py", line 707, in get_apps_json
    return json.load(f)
  File "/usr/local/lib/python3.10/json/__init__.py", line 293, in load
    return loads(fp.read(),
  File "/usr/local/lib/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.10/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 10 column 1 (char 272)

ERROR: There was a problem while creating /home/frappe/frappe-bench
Do you want to rollback these changes? [y/N]: Aborted!
error building at STEP "RUN export APP_INSTALL_ARGS="" &&   if [ -n "${APPS_JSON_BASE64}" ]; then     export APP_INSTALL_ARGS="--apps_path=/opt/frappe/apps.json";   fi &&   bench init ${APP_INSTALL_ARGS}    --frappe-branch=${FRAPPE_BRANCH}     --frappe-path=${FRAPPE_PATH}     --no-procfile     --no-backups     --skip-redis-config-generation     --verbose     --apps_path=/opt/frappe/apps.json     /home/frappe/frappe-bench &&   cd /home/frappe/frappe-bench &&   echo "$(jq 'del(.db_host, .redis_cache, .redis_queue, .redis_socketio)' sites/common_site_config.json)"     > sites/common_site_config.json &&   if [ -n "${REMOVE_GIT_REMOTE}" ]; then     find apps -name .git -type d -prune | xargs -i git --git-dir {} remote rm upstream;   fi": error while running runtime: exit status 1
ERRO[0546] exit status 1      

I am hitting this error while trying to build a custom image

@africlouds
Copy link
Contributor

I can confirm that all the migration tests were successful for me

Copy link
Contributor

@sdfateh sdfateh left a comment

Choose a reason for hiding this comment

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

I can confirm this edit builds v13 and v14 with 4 custom apps successfully and fully functional with me ( and it solved some issues that appeared with the previous build ).

Also, the migration going successful for me for all data!

We can go with these edits,
but we have to keep the previous images for any reason of use later.

@revant revant merged commit e6088af into frappe:main Jan 15, 2023
@athul athul mentioned this pull request Jan 16, 2023
1 task
ankush added a commit to frappe/bench that referenced this pull request Jan 16, 2023
* ci: fix easy-install.py test

related to frappe/frappe_docker#1032

* ci: fix easy-install.py

patched bench removed and frappe-bench installed
--no-mariadb-socket required for new-site

* fix: remove frappe version from .env

* feat(easy-install): option to set version

Co-authored-by: Ankush Menat <ankush@frappe.io>
@revant revant deleted the refactor-single-image branch January 18, 2023 07:47
@1ubuntuuser
Copy link

@revant You're the man. Thank you for simplifying this. Sorry I wasn't around to contribute took some time away over the holiday season. Single container looks great. I think what's super important for the project is newbies being able to join easily. I'll try deploying it and seeing how it runs.

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.

None yet

6 participants