Portable imports and fix for apt sources.list regarding osarch. #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…resort to --extra-filerefs=salt://docker/map.jinja,... on the cmd line. Also added the os architecture to the apt sources.list line: deb [arch=<osarch>] http://...
|
Regarding Travis, I've checked the setup of the ufw-formula and tried out something. The formula now gets executed okay, no jinja error ... however some testinfra asserts are wrong. I do not have the time to delve into that now, but probably it can be made to work with a few tweaks. travis.yml: kitchen.yml: This is the travis log: |
noelmcloughlin
approved these changes
Jul 5, 2019
There was a problem hiding this comment.
LGTM. thanks @remichristiaan If you have time please review #212 for me ;-)
docker/init.sls
Outdated
| @@ -61,6 +63,8 @@ docker-config: | |||
| - pkg: docker-package | |||
| - watch_in: | |||
| - service: docker-service | |||
| - context: | |||
| config: {{ docker.config }} | |||
There was a problem hiding this comment.
maybe config: {{ docker.config|json }} or config: {{ docker.config|yaml }} is safer.
There was a problem hiding this comment.
We need to avoid this issue: https://docs.saltstack.com/en/develop/topics/releases/2019.2.0.html#non-backward-compatible-change-to-yaml-renderer
There was a problem hiding this comment.
I'll check it with my Py3 system and will add this for py3 uses. Good catch.
There was a problem hiding this comment.
Although docs recommend |tojson the community seems to prefer |json for backwards compatibility. If you expect complicated configuration, including comments, then |yaml may be appropriate. But in this PR I believe |json is sufficient.
|
@remichristiaan this is open 11 days, so I fixed conflicts and merged to avoid this becoming stale. |
|
Sorry, was too busy last week. I’ll try to squeeze in some tests regarding the |json filter.
Met vriendelijke groet, With kind regards,
Remi-Christiaan Cool
Solution Development Manager
+31 6 13142149
Pieter Braaijweg 1 | 1114AJ | Amsterdam | The Netherlands
CC: Leiden 28065487 | Privacy statement | Terms & Conditions
From: N <[email protected]>
Sent: Friday, July 19, 2019 11:28 PM
To: saltstack-formulas/docker-formula <[email protected]>
Cc: Remi-Christiaan Cool <[email protected]>; Mention <[email protected]>
Subject: Re: [saltstack-formulas/docker-formula] Portable imports and fix for apt sources.list regarding osarch. (#216)
@remichristiaan<https://github.com/remichristiaan> this is open 11 days, so I fixed conflicts and merged to avoid this becoming stale.
The |json filter can be added in follow up PR. thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#216?email_source=notifications&email_token=AHZPIAWMZHVIM3YZET7GUDTQAIWUJA5CNFSM4H6IGSQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2MZ2TA#issuecomment-513383756>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHZPIAR2EUNNHDXUCU4IHODQAIWUJANCNFSM4H6IGSQA>.
|
|
@remichristiaan @noelmcloughlin This appears to have triggered a regression, as mentioned in #224 (comment). Probably this line here needs a default value (of Meaning, the following should resolve that problem: config: {{ docker.config | d([]) }}As you've already discussed, you may still need to append |
|
|
Hi,
I’ve tested the “| json” solution on my system and the latest master … no problems.
Summertime is busy in my line of work.
Met vriendelijke groet, With kind regards,
Remi-Christiaan Cool
Solution Development Manager
+31 6 13142149
Pieter Braaijweg 1 | 1114AJ | Amsterdam | The Netherlands
CC: Leiden 28065487 | Privacy statement | Terms & Conditions
From: N <[email protected]>
Sent: Monday, July 22, 2019 4:49 PM
To: saltstack-formulas/docker-formula <[email protected]>
Cc: Remi-Christiaan Cool <[email protected]>; Mention <[email protected]>
Subject: Re: [saltstack-formulas/docker-formula] Portable imports and fix for apt sources.list regarding osarch. (#216)
This appears to have triggered a regression
It is unhelpful that CI is broken. @remichristiaan<https://github.com/remichristiaan> please commit a PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#216?email_source=notifications&email_token=AHZPIAWL5FIX7FRQWWLLL2TQAXCE5A5CNFSM4H6IGSQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2QFLSY#issuecomment-513824203>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHZPIAWVGMIEA5FYQLZWS4LQAXCE5ANCNFSM4H6IGSQA>.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This time a version that works. Yesterday was too much of a rush job, apologies for that.
I've tested it several times with salt ssh on a clean machine, removed the thin cache this time ;)
Did not test:
Setup:
top.sls
cmd line:
Result -> succeeded
Commit msg:
Use tpldir to make imports work from e.g. salt-ssh without having to resort to --extra-filerefs=salt://docker/map.jinja,... on the cmd line.
Also added the os architecture to the apt sources.list line: deb [arch=] http://...