Skip to content

Prevent default fromrepo being evaluated as string 'None' #217

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 4 commits into from
Jun 6, 2018

Conversation

myii
Copy link
Contributor

@myii myii commented Jun 6, 2018

Discussion

@noelmcloughlin Thanks for #209; this follows-on from that.

I've been using this formula for some time with use_upstream_repo: True. Didn't have a value for fromrepo in the pillar when running this formula for a new minion. Encountered an error:

          ID: postgresql-server
    Function: pkg.installed
      Result: False
     Comment: Problem encountered installing package(s). Additional info follows:
              
              errors:
                  - Running scope as unit: run-r446ecf8982944cd4a4caa7bf54ad2e02.scope
                    E: The value 'None' is invalid for APT::Default-Release as such a release is not available in the sources

Initially tracked back to:

{% set fromrepo = repo.fromrepo or name + '-pgdg' %}

  • This was being evaluated as 'None'

Then tracked back to:

  • Used debug output of defaults.postgres to establish that the bare fromrepo: above results in {..., 'fromrepo': 'None', ...}

The proposed change in this PR allows fromrepo to be evaluated as intended.


Further potential issues

When collecting the debug output of defaults.postgres, the 'None' strings present correlate with all of the bare defaults:

{
    ...

        'homebrew': {
            'url', 'None',
            'sum': 'None'
        },

    ...

    'fromrepo': 'None',

    ...

    'linux': {
        'altpriority': 'None'
    }
}

Appears that it would be appropriate to set default values for these as well, unless there is a specific reason for using 'None' as a string.


Consideration: Add fromrepo to pillar.example

It's probably useful to have fromrepo mentioned in pillar.example. Finding an appropriate example value may be difficult. Was thinking about adding fromrepo: 'bionic-pgdg' to this PR but that is Ubuntu-specific.

Bare `fromrepo:` results in `{..., 'fromrepo': 'None', ...}`
Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

@myii good analysis. this is approved. Would be great if you can update all None to '' & update pillar.example.

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.

Good finding @myii

It seems that or comparison for non-Boolean values in Jinja are not reliable.
Although you actually fixed the issue I suggest to harden it even more and change that line:

{% set fromrepo = repo.fromrepo or name + '-pgdg' %}

Lets use default filter like this:

 {% set fromrepo = repo.fromrepo|default(name ~ '-pgdg', true) %}

@myii
Copy link
Contributor Author

myii commented Jun 6, 2018

Would be great if you can update all None to '' & update pillar.example.

@noelmcloughlin Will do that shortly. My only concern is that altpriority appears to be an integer, so the default really should be set to an integer rather than an empty string. What would be the appropriate value for that?

Lets use default filter like this:

{% set fromrepo = repo.fromrepo|default(name ~ '-pgdg', true) %}

@vutny OK, will apply that to both lines 16 and 41, which are identical.

@myii
Copy link
Contributor Author

myii commented Jun 6, 2018

@noelmcloughlin @vutny Commits pushed. I'll also push pillar.example soon. In the meantime, if there is a better suggestion for altpriority, I'll modify that and rebase.

@noelmcloughlin
Copy link
Contributor

noelmcloughlin commented Jun 6, 2018

@myii. Just set altpriority: '', thanks.

update: just saw your comment. I usually set this value (in other formulas) to 0 (zero).

@myii
Copy link
Contributor Author

myii commented Jun 6, 2018

@noelmcloughlin OK, added pillar.example and rebased to set altpriority: 0. All done from my end.

@myii
Copy link
Contributor Author

myii commented Jun 6, 2018

@noelmcloughlin Just a thought: should alternatives system be disabled by default? This won't cause regressions for users?

@noelmcloughlin
Copy link
Contributor

Yes, it should be disabled by default - definitely. 8 out of 10 people hate linux alternatives ;-)

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

Thank you, this is great work!!

@myii
Copy link
Contributor Author

myii commented Jun 6, 2018

No problem, glad to have been of assistance.

@noelmcloughlin noelmcloughlin merged commit 800259d into saltstack-formulas:master Jun 6, 2018
@myii myii deleted the patch-2 branch June 6, 2018 19:10
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.

3 participants