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

Ansible - Fix the diffing engine #1530

Merged
merged 6 commits into from
Mar 20, 2019

Conversation

rambleraptor
Copy link
Contributor

@rambleraptor rambleraptor commented Mar 15, 2019

I had to build my own differ (to determine if Ansible playbook doesn't match GCP state). It's been brittle and I've had intermittent problems. I've found some problems that I've fixed.

  1. The output-only properties make diffing hard. They should be removed. Additionally, there's no reason to send output-only fields in requests. They'll always be blank (Ansible's module validation will error if not)

  2. Python uses True/False, where GCP uses string equivalents. (Fixed this earlier)

  3. Requests use Unicode strings, responses get handed back non-unicode strings. At least in 2.7, Python is terrible about doing proper equality checks between a Unicode string and its non-Unicode equivalent (this makes perfect sense, but makes my life harder).

  4. Even without the output-only properties, the Ansible playbook state will be a subset of the GCP API results. This means I can't do something as simple as if value in list.

  5. Lists in the Ansible playbooks won't be in the same order GCP returns them.

I tried to clean up the code (to show what's the request + what's the response) and comment all of this to show a list of assumptions + why this is more complicated than one would think.


[all]

[terraform]

[terraform-beta]

[ansible]

[inspec]

@rambleraptor rambleraptor force-pushed the remove-output-only-from-response branch from eafc048 to 0be3c52 Compare March 15, 2019 20:39
@modular-magician
Copy link
Collaborator

modular-magician commented Mar 15, 2019

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of eafc04885344f67052e83bc99a107807434cb73b.

Pull request statuses

No diff detected in terraform-provider-google-beta.
No diff detected in terraform-google-conversion.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: modular-magician/ansible#216

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of 0be3c52.

Pull request statuses

No diff detected in terraform-provider-google-beta.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
Ansible already has an open PR.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@rambleraptor rambleraptor changed the title Ansible - Remove output only from response Ansible - Fix the diffing engine Mar 18, 2019
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 4953ac3.

Pull request statuses

No diff detected in terraform-provider-google-beta.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
Ansible already has an open PR.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

Copy link
Member

@SirGitsalot SirGitsalot left a comment

Choose a reason for hiding this comment

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

Would be good to take actual values from the problems you've encountered and make some unit tests from them.

@rambleraptor
Copy link
Contributor Author

@SirGitsalot ptal!

@rambleraptor rambleraptor force-pushed the remove-output-only-from-response branch from 4953ac3 to a7d6c07 Compare March 19, 2019 22:16
@rambleraptor
Copy link
Contributor Author

@SirGitsalot we had a partial test suite for the difference checker, but I added some tests to address the particular tests (resolving around sorting) that this fixes.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, a7d6c07.

Pull request statuses

No diff detected in terraform-provider-google-beta.
No diff detected in terraform-google-conversion.
No diff detected in terraform-provider-google.
Ansible already has an open PR.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@modular-magician modular-magician force-pushed the remove-output-only-from-response branch from a7d6c07 to adc5bf0 Compare March 20, 2019 17:53
@modular-magician modular-magician merged commit 0c6c2ff into master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants