Skip to content
This repository has been archived by the owner on Jan 24, 2018. It is now read-only.

[REF] Volumes management #220

Merged
merged 18 commits into from
Apr 7, 2017
Merged

[REF] Volumes management #220

merged 18 commits into from
Apr 7, 2017

Conversation

YannickB
Copy link
Owner

Hello all,

This is the PR for #219. As announced, this is a heavy refactor of the container structure, to remove the data/files/exec we used until now.

-Add new clouder.volume model. The clouder.volume is linked to service but not removed when service is unlink (unless explicit)
-Bind/Postfix/Proxy/Postgres/Odoo templates are simplified and don't use data/files/exec anymore
-SSH template is now a new application type
-Resolve some bug with raise_error having wrong number of argument
-Service now know they have to create a container thanks to an explicit checkbox, remove dummy field.

Also I officially drop the support for Odoo 8. You can still deploy Odoo 8 instance with Clouder, but you can't install Clouder module on Odoo 8 anymore. The reason is mainly the use of odoo tag in xml file.

Some image use my yannickburon docker hub repo, until the PR is merged and I can create the new image on official repo.

@codecov-io
Copy link

codecov-io commented Mar 16, 2017

Codecov Report

Merging #220 into master will increase coverage by 0.24%.
The diff coverage is 27.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   32.92%   33.17%   +0.24%     
==========================================
  Files          50       52       +2     
  Lines        3836     3901      +65     
==========================================
+ Hits         1263     1294      +31     
- Misses       2573     2607      +34
Impacted Files Coverage Δ
clouder/models/service_link.py 58.53% <ø> (+3.99%) ⬆️
clouder/models/base_metadata.py 41.66% <ø> (ø) ⬆️
clouder/models/service_metadata.py 41.66% <ø> (ø) ⬆️
clouder/models/base_option.py 83.33% <ø> (ø) ⬆️
clouder/models/base.py 23.09% <ø> (ø) ⬆️
clouder/models/base_link.py 57.14% <ø> (ø) ⬆️
clouder/models/service_option.py 85.71% <ø> (ø) ⬆️
clouder/models/image.py 42.1% <0%> (ø) ⬆️
clouder/models/template_one_2_many.py 30.3% <0%> (-0.95%) ⬇️
clouder/models/model.py 22.61% <0%> (-0.11%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59dd6ea...55d2412. Read the comment docs.

ttf-dejavu ttf-droid ttf-freefont ttf-liberation ttf-ubuntu-font-family

RUN wget https://github.com/madnight/docker-alpine-wkhtmltopdf/raw/master/wkhtmltopdf -O /usr/bin/wkhtmltopdf
RUN chmod +x /usr/bin/wkhtmltopdf
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

@lasley For the record, your Dockerfile use a wkhtmlbinary build without qt support, hence no header/footer and other random trouble. That's why I tried to find a binary on Internet compiled with both qt and alpine, this works on my instance.

Hope wkhtmltopdf provide soon a package for alpine with qt, this is quite bothersome.

@lasley
Copy link
Contributor

lasley commented Mar 28, 2017

@tedsalmon please review

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

code, no test


super(ClouderVolume, self).hook_deploy()

if not self.node_id.runner_id or \
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit easier to read using any, IMO

if any((
    not self.node_id.runner_id,
    self.node_id.runner_id.application_id.type_id.name == 'docker',
)):


super(ClouderVolume, self).hook_purge()

if not self.node_id.runner_id or \
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re any

@@ -0,0 +1,55 @@
<?xml version="1.0" encoding="utf-8"?>
<odoo>
<data>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for data tag using new odoo tag, unless you need both update and noupdate data in the same file

RUN echo "[program:php]" >> /etc/supervisord.conf
RUN echo "command=php-fpm7" >> /etc/supervisord.conf

CMD supervisord -c /etc/supervisord.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

newline at EOF

RUN echo "[program:nginx]" >> /etc/supervisord.conf
RUN echo "command=nginx" >> /etc/supervisord.conf
RUN echo "[program:php]" >> /etc/supervisord.conf
RUN echo "command=php-fpm7" >> /etc/supervisord.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO these would be more clear in a file, then use cat $FILE > /etc/supervisord.conf

Copy link
Owner Author

Choose a reason for hiding this comment

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

I agree but I'm not sure I can properly test it right now. If you don't mind, I'd rather merge it like that and correct all the Dockerfile later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with it 👍

self.deploy_links()
self.deploy_links()

for child in self.child_ids:
Copy link
Contributor

Choose a reason for hiding this comment

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

.filtered(

self = self.with_context(backup_comment='First backup')
self.backup_exec(no_enqueue=True)

for child in self.child_ids:
Copy link
Contributor

Choose a reason for hiding this comment

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

self.child_ids.mapped('child_id').recursive_backup()

@@ -1362,13 +1399,10 @@ def deploy(self):
# Create childs
if self.child_ids:
for child in self.child_ids:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use filtered

@@ -1415,6 +1452,11 @@ def hook_purge_one(self):
return

@api.multi
def purge_with_volume(self):
self = self.with_context(no_enqueue=True, purge_volume=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reassigning self is pretty hacky IMO. Why not just assign to another variable and use that?

self.stop()
# self.purge_salt()
self.hook_purge_one()

if 'purge_volume' in self.env.context:
for volume in self.volume_ids:
Copy link
Contributor

Choose a reason for hiding this comment

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

self.volume_ids.mapped('volume_id').unlink()

@YannickB
Copy link
Owner Author

YannickB commented Apr 6, 2017

Thanks @lasley ! Ready for review

@YannickB YannickB mentioned this pull request Apr 7, 2017
Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @YannickB

@YannickB YannickB merged commit 2961a4c into YannickB:master Apr 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants