Support --template-driver for docker_config (#332) #345

sashajenner · 2022-05-03T01:52:51Z

SUMMARY

Adding template_driver option with one option 'golang'. Creates a dictionary templating or None as required by create_config (https://docker-py.readthedocs.io/en/stable/api.html#docker.api.config.ConfigApiMixin.create_config). This is to be used in conjunction with a template file specified in data.

Fixes #332.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

Support --template-driver for docker_config

github-actions · 2022-05-03T02:02:33Z

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

felixfontein

Thanks for your contribution! Please add a changelog fragment.

plugins/modules/docker_config.py

sashajenner · 2022-05-04T03:47:25Z

Thank you for the detailed review. I have made updates as a result. Please let me know what you think of these changes.

sashajenner · 2022-05-04T03:52:03Z

It is possible to add some integration tests for this new option. Let me know if you think this is necessary or not.

felixfontein · 2022-05-04T05:37:15Z

It is possible to add some integration tests for this new option. Let me know if you think this is necessary or not.

This is a good idea! At least for the platforms which use the lastest Docker SDK for Python, and a new enough Docker daemon :) You simply need to add one or more tasks to tests/integration/targets/docker_config/tasks/test_docker_config.yml for that. Look at https://github.com/ansible-collections/community.docker/blob/main/tests/integration/targets/docker_container/tasks/tests/options.yml#L1046-L1084 for an example of a test which only works for certain Docker daemon and Docker SDK for Python versions: it ignores failures for all tasks, and then checks whether it behaved correctly.

felixfontein

I think self.data_key needs to be updated when templating is used. I'm not sure what's the best way to do this. First you should probably add a method def _data_key_content(self): which is used in line 255 as self.data_key = hashlib.sha224(self._data_key_content()).hexdigest().

In that function, I would probably compile a configuration dictionary that's empty when template is not specified, and otherwise contains a key template_driver with the value of the template_driver option. If that config dict is empty, return data. Otherwise, return data + SALT + json(config_dict) (with sort_keys etc. so that JSON generation does not depend on platform), where SALT is a constant to be created containing a fixed but random string. (For example nu7Zook2bo4Ohyah1ieghae2ohpho7Haa1yeefieNgoo9ov3que3yaiKeing1poh.)

felixfontein · 2022-05-05T11:23:53Z

Hmm, it would probably be better if you could determine for an existing config which templating method is used. If that is doable, use that to fix idempotence and leave self.data_key alone.

felixfontein · 2022-05-05T11:24:52Z

You'll have to add the idempotence check in

community.docker/plugins/modules/docker_config.py

Lines 334 to 347 in 26a61e9

    
           self.results['config_id'] = config['ID'] 
        
           self.results['config_name'] = config['Spec']['Name'] 
        
           data_changed = False 
        
           attrs = config.get('Spec', {}) 
        
           if attrs.get('Labels', {}).get('ansible_key'): 
        
               if attrs['Labels']['ansible_key'] != self.data_key: 
        
                   data_changed = True 
        
           else: 
        
               if not self.force: 
        
                   self.client.module.warn("'ansible_key' label not found. Config will not be changed unless the force parameter is set to 'yes'") 
        
           labels_changed = not compare_generic(self.labels, attrs.get('Labels'), 'allow_more_present', 'dict') 
        
           if self.rolling_versions: 
        
               self.version = self.get_version(config) 
        
           if data_changed or labels_changed or self.force:

sashajenner · 2022-05-10T05:08:43Z

I think I've added an idempotency check now. It seems to work in testing. That is, when everything is the same but template_driver is modified, ansible says the task has changed.

However, I'm not confident in my solution. In particular, I'm not sure if I need to do a check as in

community.docker/plugins/modules/docker_config.py

Line 320 in 98b2270

if attrs.get('Labels', {}).get('ansible_key'):

for template_driver as well.

felixfontein · 2022-05-10T05:33:08Z

plugins/modules/docker_config.py

            attrs = config.get('Spec', {})
            if attrs.get('Labels', {}).get('ansible_key'):
                if attrs['Labels']['ansible_key'] != self.data_key:
                    data_changed = True
            else:
                if not self.force:
                    self.client.module.warn("'ansible_key' label not found. Config will not be changed unless the force parameter is set to 'yes'")
+            if attrs['Labels']['template_driver'] != self.template_driver:


This should likely be something like:

Suggested change

if attrs['Labels']['template_driver'] != self.template_driver:

if attrs.get(...) != self.template_driver:

Unfortunately the API docs (https://docs.docker.com/engine/api/v1.41/#operation/ConfigInspect) do not give any hint how and where that information is available. Can you paste the output of something like docker config inspect ... for such a config?

In any case, this is one more reason why there should be tests for this in CI :)

For this docker config task

- name: debug community.docker.docker_config: name: db_password data: opensesame! state: present template_driver: golang

the output of docker config inspect ... looks like this

[root@447f901529a4 /]# docker config inspect db_password [ { "ID": "ilwv23ojrt2ooisz06enkmovj", "Version": { "Index": 28681 }, "CreatedAt": "2022-05-11T01:45:58.5768652Z", "UpdatedAt": "2022-05-11T01:45:58.5768652Z", "Spec": { "Name": "db_password", "Labels": { "ansible_key": "4c8ed891d2bcad5bd4359a985e82349ec42e0c04be5511b0e53ac258" }, "Data": "b3BlbnNlc2FtZSE=", "Templating": { "Name": "golang" } } } ]

Hence, I now access the attrs['Templating']['Name'] to get a config's template_driver. I have done some local testing and it seems to work. I will now work on adding integration tests.

codecov · 2022-05-10T09:27:27Z

Codecov Report

Merging #345 (98b2270) into main (51d4c55) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 98b2270 differs from pull request most recent head 8cad235. Consider uploading reports for the commit 8cad235 to get more accurate results

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
- Coverage   73.23%   73.21%   -0.03%     
==========================================
  Files          59       59              
  Lines        8167     8168       +1     
  Branches     1885     1885              
==========================================
- Hits         5981     5980       -1     
- Misses       1683     1685       +2     
  Partials      503      503

Flag	Coverage Δ
integration	`72.86% <100.00%> (-0.06%)`	⬇️
sanity	`17.03% <0.00%> (-0.01%)`	⬇️
stub	`0.00% <0.00%> (ø)`
units	`27.58% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugins/modules/docker_swarm.py	`81.81% <100.00%> (-0.73%)`	⬇️

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 51d4c55...8cad235. Read the comment docs.

felixfontein

LGTM

felixfontein · 2022-05-11T05:27:11Z

@sashajenner thanks for contributing this!

Sasha Jenner added 3 commits April 27, 2022 14:29

initial templating changes

c96f90c

adding docs

92f08dd

author

3652e14

felixfontein reviewed May 3, 2022

View reviewed changes

plugins/modules/docker_config.py Show resolved Hide resolved

plugins/modules/docker_config.py Outdated Show resolved Hide resolved

plugins/modules/docker_config.py Outdated Show resolved Hide resolved

plugins/modules/docker_config.py Outdated Show resolved Hide resolved

felixfontein added enhancement New feature or request docker-swarm Docker Swarm labels May 3, 2022

responding to pr comments

26a61e9

felixfontein reviewed May 5, 2022

View reviewed changes

Sasha Jenner added 4 commits May 10, 2022 11:54

testing for idempotency

0dc3856

using template_driver rather than templating

290cb2c

adding explicit idem check

81d08ce

remove local testing changes

465573b

bug fix

50ba6a5

felixfontein reviewed May 10, 2022

View reviewed changes

fixing idempotency

8cad235

felixfontein approved these changes May 11, 2022

View reviewed changes

felixfontein merged commit c63ef41 into ansible-collections:main May 11, 2022

felixfontein mentioned this pull request May 11, 2022

docker_config integration tests for template_driver #347

Merged

wyoa-bot bot mentioned this pull request May 14, 2022

feat(ansible): update dependency community.docker to v2.5.0 jr0dd/home-ops#1123

Merged

1 task

This was referenced May 14, 2022

chore(deps): update dependency community.docker to v2.5.1 linxside/ansible-ee#16

Merged

chore(deps): update dependency community.docker to v2.6.0 kashalls/home-cluster#624

Merged

negative-7-bot bot mentioned this pull request May 17, 2022

feat(ansible): update dependency community.docker to v2.5.1 negative7/home-cluster#823

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support --template-driver for docker_config (#332) #345

Support --template-driver for docker_config (#332) #345

sashajenner commented May 3, 2022

github-actions bot commented May 3, 2022 •

edited

Loading

felixfontein left a comment

sashajenner commented May 4, 2022

sashajenner commented May 4, 2022

felixfontein commented May 4, 2022

felixfontein left a comment

felixfontein commented May 5, 2022

felixfontein commented May 5, 2022

sashajenner commented May 10, 2022

felixfontein May 10, 2022

sashajenner May 11, 2022 •

edited

Loading

codecov bot commented May 10, 2022 •

edited

Loading

felixfontein left a comment

felixfontein commented May 11, 2022

	if attrs['Labels']['template_driver'] != self.template_driver:
	if attrs.get(...) != self.template_driver:

Support --template-driver for docker_config (#332) #345

Support --template-driver for docker_config (#332) #345

Conversation

sashajenner commented May 3, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

github-actions bot commented May 3, 2022 • edited Loading

Docs Build 📝

felixfontein left a comment

Choose a reason for hiding this comment

sashajenner commented May 4, 2022

sashajenner commented May 4, 2022

felixfontein commented May 4, 2022

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented May 5, 2022

felixfontein commented May 5, 2022

sashajenner commented May 10, 2022

felixfontein May 10, 2022

Choose a reason for hiding this comment

sashajenner May 11, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented May 10, 2022 • edited Loading

Codecov Report

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented May 11, 2022

github-actions bot commented May 3, 2022 •

edited

Loading

sashajenner May 11, 2022 •

edited

Loading

codecov bot commented May 10, 2022 •

edited

Loading