-
Notifications
You must be signed in to change notification settings - Fork 303
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
[IMP] Add option to automatically install repo requirements #28
Conversation
38cbe5b
to
70c0b91
Compare
But installing the requirements.txt from the repo if you're not going to use all the modules is a bit abusing. Can you read external_dependencies from the module manifests instead? |
@yajo what do you think about this overhead? |
This is mainly meant for dev boxes, where I don't want to have to worry about my s/w engineers dealing with Ops (same use case as #27). I would never recommend either of these to be used in a production instance. Honestly the real solution is an auto-installer in Odoo based on the |
OK, then you should activate this by an environment variable only set on devel.yaml, not for all. |
The
I'll be setting that (
Oh I didn't even think about updating the scaffold too. I'll submit a PR for that once this is merged! |
OK, I'm not a Docker expert, so if doing that then you can decide later on yaml files, then go ahead. Sorry for the noise. |
Problem is that for
I don't feel that this is a good idea. Your developers should ensure the image includes required dependencies, or chances to get a red production build will be high. Our approach with this project tries to be: provide tools that make developing easy and fast, and make production reproductible and stable. Adding autodiscovery in devel and removing it in prod adds chances to get different environments, so I'm not very positive with this feature. OTOH, #28 (comment) would be awesome, but there's still the problem of #26, so we need a way to handle all of that. pip-installing in entrypoint is not an option IMHO (too slow for a boot process). Any ideas? |
My developers don't have any idea what is done on a production server, including how to make it green or red. My goal here is to make it so that my devs only have to worry about the code they are currently working in, and not how to install dependencies correctly for modules that are required by other modules they may use. We have a completely separate process for building out production instances, which would be a combination of many different modules that these developers are making. This process would not use wildcards, and any issues would be detected during that QA. The two processes create a logical isolation between our development and ops departments, which makes it easier to onboard engineers (particularly juniors). Sure, I get that any Python developer should know how to pip install something. They should not, however, be required to know how to track down which virtualenv the Odoo instance is running in, and how to pip install things to it & not system Python. They should also not be required to know how to modify a Docker file & rebuild their instance in order to dev for Odoo. It is also a hardline requirement to have all dev instances operating under the same architecture as our prod ones, so simply having them install local Odoos is not an solution either.
I don't understand the problem here? Isn't the Is there a key in the
I am not proposing to change the way anything operates, I am merely stating a use case. IMO it is not up to us to determine whether someone wants to do this on a production instance or not. My use case is devel, and is recommending against using in prod, but there is nothing to stop you from doing so.
Yeah so I definitely feel like I'm missing something here. I think answers to above question about aggregate key might clue me in.
Agreed - also this goes against what I've read of Docker practices. |
38cbaae
to
aef2dcb
Compare
Followup now that I dug more for #27 - I totally get the problem now, so we can probably disregard most of my above. Regarding the use of the manifest keys - I don't know of a reliable way to translate the Python Package we are provided in the manifest into to the PIP library that needs to be installed, do you? And back to when we can perform this installation - I really don't like the entrypoint, because that's mutating the container after creation. We need something in the build process so that it can make its way into the images. What if this feature were to be moved into the repo aggregation step instead? It's implicitly bound to this step, so it makes sense for the cutoff to be here. Looking at the scaffolds, it seems like the repo aggregation is the entrypoint as a setup script, so the installation would be performed there as well. |
🤔 It could be at aggregation time, yes... But what if you have an additional dependency in your Besides, current design allows you to choose your pip-freezing method: Do you want to pin by version? Hash? not version-pinning? Do you want to install addons with pip instead of
So all you need, you write it there and all gets added at build time without the need of custom build scripts, no matter if you build for devel or prod... Current problems in our dev team that would get fixed by this autodiscovery would be avoiding this:
Not much hassle really... But still I'd want to cover your case if possible without losing current feature set, so what about this? What if we add a new script There's another option: always copy the code, also in devel, so you can inspect and install, even if later you replace that path by a volume. But that would really slow down devel builds, and quick building there is a requirement IMHO. |
aef2dcb
to
7d6aa0c
Compare
81bc015
to
4c2c99a
Compare
- More instructions on this feature. - More instructions on ssh keys too. - More DRY with FILE_APT_BUILD. - Useful logging on dependencies installation commands. - Some style homogeneization. - Move install module inside odoobaselib namespace. - Fix #35 by giving everyone read access to odoobaselib.
6fee3d4
to
7e825d7
Compare
Ok so I've been thinking about this, and I'm not sure of a way that this feature can be supported in development environments that are not linking the addons during build time. I don't think that this is a good reason to not support it in the other case though. Honestly I'm still even finding it easier in my dev environments to link the addons during build, then just mount my specific working addon as a volume. In this way, I don't have to worry about 5 other repos being on the correct branches & instead we just use the same images that would be deployed to our customer prods. |
I feel you shot yourself in the toe in #27 😕 . Now the addons get linked at run time, not build time, so not even production builds have a notion of "enabled addons". And pip-installing in entrypoint is out of question IMO. What would be your implementation strategy now on this? Besides, I think I was a little bit wrong before. Devel images get addons in too, but they just get replaced by volumes at run time. I'm thinking on a simple approach: add a script that outputs a valid
The script works in run time after all entrypoints are processed, and the workflow produces git-versioned image-frozen dependencies, so it seems to cover all scenarios in a semi-automatic fashion that makes your life easier but still lets you have the control of the tool and is fully retrocompatible. |
@yajo - I'm not sure what you mean, addons are definitely still linked during build time. This doesn't involve the linking of the addons though, only the download of them. It's probably worth noting that this works fine on my forked image, which has had all of these features for a while. The requirements implementation is dumb - it doesn't matter what addons are enabled, just that the addon header is in the addons file. We can't determine which of the requirements goes to which module, so selectively installing isn't going to work. I would prefer to have the requirements installed in my built image, as opposed to building them in on entrypoint. The entrypoint solution still involves a developer, which I am eliminating from this equation. These instances should self maintain IMO, including dependency installations as new requirements are added to repos & new addons are introduced to pre-existing instances that didn't previously have them. Requiring that another step be performed by an engineer after deploying my images is not really an option on my end. |
OK, I was just warning you about some problems I was seeing on this approach, but if you have been able to sort them out, there's no problem. Just push a valid PR with regression tests and let us review 😊 (this one has conflicts) |
7e825d7
to
db55e9e
Compare
I think the Travis is unrelated here too
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you were going to investigate external_dependencies
on addons found in addons.yaml
, not to read the requirements.txt
files possibly in repos.
In any case, it seems legit as a simple 1st approach.
Please rebase to see if travis goes green.
Also, could you add a test please?
build.d/100-dependencies
Outdated
req_file = os.path.join( | ||
SRC_DIR, repo, 'requirements.txt', | ||
) | ||
if os.path.isfile(req_file): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EAFP
build.d/800-auto-requirements
Outdated
@@ -0,0 +1,16 @@ | |||
#!/usr/bin/env python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this file.
I haven't found a way to conquer this, but I am absolutely all ears if you have one. My issue is described here, but the TL;DR is that the PIP packages do not align with the Python namespaces they expose. We need to install the PIP packages, but the Odoo manifest uses the exposed namespaces. |
db55e9e
to
1e3e87f
Compare
e4c1d0f
to
4430752
Compare
- Shortcut for installation that reduces the logic size. - Cannot install from repositories before aggregation. - Auto dependencies should come in before manual ones. - Drop support of deprecated /opt/custom/src/requirements.txt file.
4430752
to
a3cdb78
Compare
I'm reverting this as it causes images to not build, that we didn't anticipate. The problem is the following:
We should resubmit this thinking about this. |
@@ -18,6 +18,7 @@ ONBUILD ARG DEPTH_MERGE=100 | |||
ONBUILD ARG CLEAN=true | |||
ONBUILD ARG COMPILE=true | |||
ONBUILD ARG CONFIG_BUILD=true | |||
ONBUILD ARG AUTO_REQUIREMENTS=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have been false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as a good sane compatibility default, but anyway the problem is still there, and you will face it when you set this param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more for a development box, and while it can cause issues it also is incredibly helpful (at least it has been for me).
Is there any reason to not leave this feature as is, just with the good default? This is one of the last things I need to move off of my fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issues are during build too, which is the perfect time to resolve them. It really isn't a problem when you're setting up the box, only when a bunch of pre-setup boxes get the wrong default IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, please resend the PR with that defaulting to False.
This is a WIP & is temporarily based on #27 while I am testing, because I'm trying to be ultra lazy.
This PR adds an
AUTO_REQUIREMENTS
arg (currently defaults to True while I'm testing, but meant to be False). If the argument is true, requirements.txt files from addons defined in theaddons.yml
will be honored automatically.As an added bonus,
80-requirements-txt
is now Pythonified