Skip to content

use upstream repo for python if configured #204

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

Merged
merged 2 commits into from
Jun 9, 2018

Conversation

noelmcloughlin
Copy link
Contributor

For some reason python.sls was missing upstream repo support. PR to fix that.

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

Hi @noelmcloughlin . This needs to be rebased though...

Despite that, as a matter of consistent style, I would suggest to avoid putting include statements somewhere other than at the top of SLS file. This is declarative and imply mental model of state execution order.
At the same time, to reduce duplicates of Jinja statements, it is safe to just include postgres.upstream unconditionally. The logic deciding to configure the repo or not is hidden in the upstream.sls anyway, and if that is disabled/not applicable -- it would render just nothing.

That would really improve the readability of the file and make the states less coupled.

@noelmcloughlin noelmcloughlin force-pushed the python branch 2 times, most recently from 1e5b3db to 7634296 Compare May 17, 2018 15:24
@noelmcloughlin
Copy link
Contributor Author

noelmcloughlin commented May 17, 2018

Thanks for taking time to review @vutny. Rebased. I implemented your review comments.

However, server/init.sls was my reference point for need to conditionally include: - upstream.sls. But since we agree a conditional include of upstream.sls is superfluous, a future PR should address this in server/init.sls too - just to be consistent across SLS files.

server/init.sls

{%- set includes = [] %}
... 
{%- if postgres.use_upstream_repo == true -%}         <========= not needed
  {%- do includes.append('postgres.upstream') %}
{%- endif %}               <=== not needed

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

This is good to go @noelmcloughlin . Thanks.

@noelmcloughlin noelmcloughlin merged commit 6840bf3 into saltstack-formulas:master Jun 9, 2018
@noelmcloughlin noelmcloughlin deleted the python branch June 9, 2018 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants