Skip to content
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

Fix broken rendering debian_eth.jinja. #46980

Merged
merged 3 commits into from Jul 6, 2018

Conversation

Projects
None yet
3 participants
@MTecknology
Copy link
Contributor

commented Apr 9, 2018

Space-delimited values are passed as strings and not iterable.
This caused the string to be exploded as a list of single characters.

The end result was every state execution resulting in the following changes:

----------
          ID: net-eth0
    Function: network.managed
        Name: eth0
      Result: True
     Comment: Interface eth0 updated.
     Started: 22:32:18.811798
    Duration: 10.185 ms
     Changes:   
              ----------
              interface:
                  --- 
                  +++ 
                  @@ -4,6 +4,6 @@
                       netmask 255.255.254.0
                       gateway 10.x.0.1
                       mtu 9000
                  -    dns-nameservers 1 0 . x . 0 . 7   1 0 . x . 0 . 8
                  -    dns-search  s u b . d o m a i n . t l d   d o m a i n . t l d
                  +    dns-nameservers 10.x.0.7 10.x.0.8
                  +    dns-search sub.domain.tld domain.tld

This patch modifies the template to not iterate over the passed value for two options (dns-nameservers, dns-search) could previously handle concatenating a list of values.

It's possible the module could be modified to accept a list and turn it into a string, but that seems less important than fixing this bug, which appears to be consistently broken.

Fix broken rendering debian_eth.jinja.
Space-delimited values are passed as strings and not iterable.
This caused the string to be exploded as a list of single characters.
@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2018

Nice! Any chance you'd be willing to write a test for this so we don't regress in this area again?

Mike Place
@MTecknology

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2018

I wouldn't call this one a regression, rather, I don't think it's ever worked correctly. Could you point me in the right direction for learning how to write a test case for jinja rendering? I've attempted this in the past but every attempt has warped my fragile mind into a state of confusion.

I'd like to be able to write a unit test that includes creating and writing a dummy interfaces file, to ensure reading/parsing is handled correctly.

As far this particular patch goes, it works, but I'd like to work on it a bit more. I want a solution that can handle both lists /and/ strings. I thought that would be easier than what it's turning out to be, but I'll continue this pursuit.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented May 31, 2018

Hey @MTecknology - there are some docs about writing tests here: https://docs.saltstack.com/en/latest/topics/development/tests/index.html. Hopefully that helps!

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

Hi @MTecknology - just a bump here. Did you get a chance to write some tests for this?

@MTecknology

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

@rallytime Thanks for those documentation links!

I had some rather major surgery recently and I'm pretty far from recovered, so this has been sitting on the back burner on a very low simmer.

To be perfectly honest, this area of code is kind of a nightmare (as previous commit messages would indicate...). When I'm feeling better, I feel like I need to take a step back, get a more clear high/low level understanding of how salt utilizes these functions, and come up with something substantially more robust. This is a can of dragons that I don't really want to open, but I think it needs to be opened and tamed. Once tamed, I think it will become substantially easier to write unit tests.

I haven't let this drop out of my inbox! I will eventually put on my armor and tame these beasts!!! 🗡

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

@MTecknology Thanks for the explanation, and I agree, some more comprehensive unittests are needed for this area. I think overall this is a simple patch and really we just wanted to make sure this doesn't regress again. I think we can get this in now, but I'd love to see the more robust idea (complete with unittests) that you come up with when you're feeling better.

Good luck on your recovery! Thank you for contributing. :)

@rallytime rallytime merged commit 776912a into saltstack:develop Jul 6, 2018

7 of 10 checks passed

default Pull Requests » Salt PR - Main Build - PY2/PY3 #9984
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
codeclimate All good!
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.