Introduce fromrepo support. #209
Conversation
There was a problem hiding this comment.
Well done, @noelmcloughlin !
I just have couple of things to address. Despite of that, your changes look solid.
postgres/codenamemap.yaml
Outdated
| {% set pkg_dev = '' %} | ||
| {% else %} | ||
| {% set fromrepo = name %} | ||
| {% set pkg_dev = 'postgresql-server-dev-all' %} |
There was a problem hiding this comment.
I think you want to expose pkg_dev value in the rendered YAML dictionary below, since it would be just gone. Simply because upper level yaml loader does not inherit local variables, especially within macros scope.
There was a problem hiding this comment.
Thanks for reviewing @vutny I'll have a look at the comments now.
There was a problem hiding this comment.
I just realized pkg_dev was already fixed in osfamilymap.yaml by #200. I think we could remove pkg_dev from codenamemap.yaml?
| pkg_repo: | ||
| name: 'deb http://apt.postgresql.org/pub/repos/apt {{ name }}-pgdg main {{ repo.version }}' | ||
| name: 'deb http://apt.postgresql.org/pub/repos/apt {{ name }}-pgdg main {{ version }}' |
postgres/defaults.yaml
Outdated
| @@ -57,6 +57,10 @@ postgres: | |||
|
|
|||
| bake_image: False | |||
|
|
|||
| fromrepo: | |||
| pkg_repo: | |||
| name: [] | |||
There was a problem hiding this comment.
This is a little bit dangerous and I would recommend to avoid setting such defaults. I mean pkg_repo dictionary.
- Empty name is meaningless (why the list by the way?) and still will produce sneaky errors if other maps would not set it right.
- If
use_upstream_repoPillar is set, theupstreamstate would always try to provision some repo config, even if that is not applicable for some systems.
There was a problem hiding this comment.
Thanks for the assessment - the from_repo will work for both upstream and distro postgres.
do you think this stanza is unnecessary in defaults.yaml? Thanks.
There was a problem hiding this comment.
Only the pkg_repo is harmful, because it would interfere with repository config handling logic.
Keeping the fromrepo is safe and necessary though.
|
These are resolved now. |
|
Thanks again @vutny |
This PR introduces
fromreposupport (replaces #185 and resolves #133). Testing was done on Ubuntu/Centos/Fedora. Comments welcomed.