repo_config improperly setting as str breaking config.repo #161
Conversation
|
@mdellweg - pinging to review. When is v0.16 expected to be released? Thanks |
plugins/modules/rpm_repository.py
Outdated
| if repo_config is not None and isinstance(repo_config, string_types): | ||
| desired_attributes["repo_config"] = json.loads(repo_config) |
There was a problem hiding this comment.
What happens if it's not a string?
I'm unsure how this is supposed to be represented. It looks like we are missing either case in both versions. We should have a test for this.
"A JSON document or data structure describing a config.repo file"
There was a problem hiding this comment.
-
As a string, it's converted to a json blob
repo_config: '{"gpgcheck": 1}' -
As a hash of key:pairs, ansible properly converts this to a json blob and passes to the API.
repo_config:
gpgcheck: 1
anotherkey: value
anotherkey2: value
In either of these cases, it'll work. The current code test for the (1) string value, should we have a test for (2) format as well?
There was a problem hiding this comment.
ATM desired_attributes is only set on one branch of the if.
So yes, I could really be convinced by seeing both versions in a test.
There was a problem hiding this comment.
@mdellweg - These tests are already in place, just the fixtures were not proper. (I'm now remembering)
See: https://github.com/pulp/squeezer/blob/develop/tests/playbooks/rpm_repository.yaml#L115-L161
desired_attributes is only set on one branch of the if
desired_attributes is initially set with this code:
https://github.com/pulp/squeezer/blob/develop/plugins/modules/rpm_repository.py#L127-L129
The IF statement is modifying the desired_attributes["repo_config"] to correctly format the JSON blob when a string vs dict is provided.
There was a problem hiding this comment.
I see.
Then let's make this more obvious and fix desired_attributes["repo_config"] in place without reading from module.params again.
There was a problem hiding this comment.
It would also be ok to just require it to be parsed from json already. I think there is a "from_json" filter in ansible to bridge the gap.
There was a problem hiding this comment.
Removed the rewriting from module.params and in place fix for desired_attributes.
There was a problem hiding this comment.
Can you change the test in a way, that the module is faced with both versions once?
There was a problem hiding this comment.
I've changed the check to test the json version and then the string version, validating no changes to the repo are detected. This confirms different endpoints produce the same output.
chtaylo2
force-pushed
the
develop
branch
2 times, most recently
from
ff4e1ab
to
f8672e6
Compare
plugins/modules/rpm_repository.py
Outdated
| if module.params["repo_config"] is not None and isinstance( | ||
| module.params["repo_config"], string_types | ||
| ): |
There was a problem hiding this comment.
Let's try to stay in the domain of the problem here too (We should be done with module.params at this point):
| if module.params["repo_config"] is not None and isinstance( | |
| module.params["repo_config"], string_types | |
| ): | |
| if "repo_config" in desired_attributes and isinstance( | |
| desired_attributes["repo_config"], string_types | |
| ): |
There was a problem hiding this comment.
Sure. I've made this change as requested
|
@mdellweg - What is the release cadence for Squeezer? Thanks |
fixes #153