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

Making casting optional #10

Closed

Conversation

benpilletspoken
Copy link
Contributor

No description provided.

@elmarx
Copy link
Owner

elmarx commented Feb 16, 2018

Hm, what is your use case for this, or what's the issue with the "automatic" casting?

To be honest, that option looks a little bit ugly, and disables casting for all values, if e.g. it's required for a specific column only.

Maybe the whole casting isn't necessary at all, if the playbook sets proper types (which should be possible, looking at ansible/ansible#15249).

And, besides that I'd like to add options to the example/documentation, so I need to understand why it's required :)

@benpilletspoken
Copy link
Contributor Author

Good questions.
I'm working with a database that has a string column for configuration values. Of course, we had to put a number in there for one of the options. When the comparison is done here: https://github.com/zauberpony/ansible-mysql-query/blob/master/library/mysql_query#L119
the expected_query_result has a member that was cast to a long as 101L and the actual_result has a string with '101'.
So, the automatic casting makes it so I can't specify in my task what the type of the expected_query_result should be. I can't do this: config_value: '101'. I tried different types in the task, and they call got cast to a long here: https://github.com/zauberpony/ansible-mysql-query/blob/master/library/mysql_query#L217
As for potential solutions, I agree that cast_values is kinda ugly. And, there should be a way of doing it per value. Removing the casting all together might break things for existing code bases, but it's also probably the right thing to do for the future. My vote is to remove it all together and have the operator use the correct type in the task. Second option would be to make the type optional for each expected value in the task and cast both the expected and actual values before the comparison.

@elmarx
Copy link
Owner

elmarx commented Feb 16, 2018

By "operator" you mean the person writing the playbook? Then we both agree that we should get rid of the casting.

Right now I'm fixing all the tests, they stopped working with ansible 2 (?) when it removed ansible.runner. I found some examples for (unit-) tests in the ansible-codebase, and I already fixed the first file.

Once I have a test-environment set up, it's much easier for me to try out the possible options, and based on the knowledge gained from that we could decide which option is best to support your use case.

An additional option is to cast conditionally based on type-information gathered from the database itself, as I just found in a fork: koichirok@45e04ee

That seems to be a valid solution, too.

@elmarx
Copy link
Owner

elmarx commented Feb 19, 2018

Hey, can you have a look at the experimental-branch (97ecd04)?

My idea was to have equality-testing with type-coercion/implicit casting, just like JS's ==. Turns out it's not that easy with python… so my lazy solution was to possible convert ints/floats to string and compare that with the desired value.

It works for my test-case and work in manual testing, so if it work for you, too, I would merge it to master and release a new version.

@benpilletspoken
Copy link
Contributor Author

Works on my setup 👍 Merge away

@elmarx
Copy link
Owner

elmarx commented Feb 23, 2018

Thanks, now in master and tagged.

@elmarx elmarx closed this Feb 23, 2018
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.

None yet

2 participants