magic-modules icon indicating copy to clipboard operation
magic-modules copied to clipboard

Add additional loop to check changes from resp list

Open dabde opened this issue 3 years ago • 4 comments

Currently by tests, I found out that "gcp_compute_firewall" is not working like expected. Existing "allowed" ports, didn't get deleted and no change was detected. After some investigation, found that the comparison for "is_different" is not working correct.

https://github.com/ansible-collections/google.cloud/blob/master/plugins/modules/gcp_compute_firewall.py#L688

More steps and I was able to reproduce the comparison and see the order of the comparison is important.

>>> resp = {u'name': u'test-fw-module', u'sourceRanges': [u'127.0.0.1', u'10.10.10.10/29'], u'priority': 1000, u'targetTags': [u'monitor'], u'allowed': [{u'IPProtocol': u'tcp', u'ports': [u'1255']}, {u'IPProtocol': u'tcp', u'ports': [u'12435']}], u'network': u'https://www.googleapis.com/compute/v1/projects/kci-wordpress/global/networks/default'}
>>> req = {u'name': 'test-fw-module', u'sourceRanges': [u'127.0.0.1', u'10.10.10.10/29'], u'priority': 1000, u'targetTags': ['monitor'], u'allowed': [{u'IPProtocol': 'tcp', u'ports': ['1255']}], u'network': 'https://www.googleapis.com/compute/v1/projects/kci-wordpress/global/networks/default'}
>>> gcp_utils.GcpRequest(resp) == gcp_utils.GcpRequest(req)
False
>>> gcp_utils.GcpRequest(req) == gcp_utils.GcpRequest(resp)
True

I don't know if this code is the best solution, but was resolving for me the wrong behavior of this module. Related this is a basic object for the ansible google cloud module, I'm not aware if this can cause other side effects.

refs also to this issue: https://github.com/ansible-collections/google.cloud/issues/402

If this PR is for Terraform, I acknowledge that I have:

  • [ ] Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • [ ] Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • [ ] Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • [ ] Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • [ ] Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

Fixed list comparison in gcp_utils.py to check both side of a GcpRequest object.

dabde avatar Jun 15 '21 16:06 dabde

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@c2thorn, please review this PR or find an appropriate assignee.

modular-magician avatar Jun 15 '21 16:06 modular-magician

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform Beta: Diff ( 2 files changed, 2 insertions(+), 3 deletions(-)) Ansible: Diff ( 1 file changed, 11 insertions(+))

modular-magician avatar Jun 15 '21 16:06 modular-magician

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=192184"

modular-magician avatar Jun 15 '21 16:06 modular-magician

Forwarding this to @rambleraptor since this is relating to Ansible

c2thorn avatar Jun 16 '21 18:06 c2thorn