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

Backend improvements #87

Closed
wants to merge 3 commits into from
Closed

Backend improvements #87

wants to merge 3 commits into from

Conversation

erral
Copy link
Member

@erral erral commented Oct 27, 2024

Some backend improvements:

  • Why are we creating the docker image with a specific requirements-docker.txt file instead of the requirements.txt file? I think that that makes everything more complex. I was hit by this, and I just did not understand what's going on: I added some dependencies to requirements.txt on development, build the docker image, deploy it, and my dependencies were missing. Why not use the same file to build the docker image? That's what I do in the PR.
  • Running the update_locale script installed in the virtualenv is wrong. What if several packages install the same script? Which one wins? Nobody knows! I was hit by this too: I had a package installed as a dependency (collective.contentrules.telegram), which is a package generated with bobtemplates.plone boilerplate which also installs the update_locale script in bin (btw, the backend_addon also does this), so when running make i18n in my backend add-on, the installed one was executed, and in my case the collective.contentrules.telegram one was executed. I guess that when running make i18n in our backend folder, we want to update our own package's locales! That's what this PR does, calling update.sh in this very add-on.

@erral erral requested a review from sneridagh as a code owner October 27, 2024 06:43
@erral erral requested review from ericof and davisagli October 27, 2024 06:43
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Member

@ericof ericof left a comment

Choose a reason for hiding this comment

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

About the requirements-docker.txt, ok.
But the update_locale I would go on a different route. (There is an issue about this opened here)

@erral
Copy link
Member Author

erral commented Nov 11, 2024

I will open a PR removing the requirements-docker.txt

I talked to Érico and he has some ideas about the update_locales scripts, so I will leave that out of the PR.

@davisagli
Copy link
Member

Sounds like we should close this one then.

@davisagli davisagli closed this Nov 12, 2024
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.

3 participants