search

infobloxopen / infoblox-ansible

Ansible modules for interfacing to Infoblox systems
GNU General Public License v3.0
54 stars 60 forks source link

Adding module for Extensible Attributes object #186

Closed matthewdennett closed 2 months ago

matthewdennett commented 1 year ago

This PR is to add the functionality of managing extensible attributes.

This will address feature request: https://github.com/infobloxopen/infoblox-ansible/issues/179

matthewdennett commented 1 year ago

Hey @hemanthKa677 can you please review this pull request?

matthewdennett commented 10 months ago

@ranjishmp @hemanthKa677 any chance this can get a review?

matthewdennett commented 9 months ago

@ranjishmp do you have a rough time frame on when that will be implemented?

ranjishmp commented 9 months ago

@matthewdennett @lkanthinfoblox is managing this now.

hmalphettes commented 3 months ago

@lkanthinfoblox I am also interested in this PR - thanks @matthewdennett for submitting it and for all the other contributions!

I had no problem rebasing this PR on the top of the latest codebase in my github. It is on my github fork. I am able to configure or update extended attributes with this.

I would like to figure out how to configure the additional properties too eventually.

Let me know if there is anything I can do to help.

matthewdennett commented 3 months ago

@lkanthinfoblox, is it possible that this one be looked at please?

lkanthinfoblox commented 3 months ago

@hmalphettes and @matthewdennett, We started looking into this PR. We will update the outcome in couple of days!

matthewdennett commented 3 months ago

@lkanthinfoblox, thanks for the reply.

What can we do in the future to avoid waiting ~1 year for a review? Happy to discuss in the discussion thread here if that suits better? - https://github.com/infobloxopen/infoblox-ansible/discussions/150#discussioncomment-9569359

JkhatriInfobox commented 3 months ago

Hello @matthewdennett, I noticed that your pull request has been open for a while. Could you please update it with the latest changes from the master branch and resolve any conflicts if they exist? This will help us to review and merge your changes more efficiently.

matthewdennett commented 3 months ago

@JkhatriInfobox, fingers crossed I didn't break anything with that mege.

hmalphettes commented 3 months ago

Wow, thank you both.

The linting run by ansible sanity tests are reporting some missing new lines:

ERROR: Found 2 pep8 issue(s) which need to be resolved:
ERROR: plugins/module_utils/api.py:217:1: E302: expected 2 blank lines, found 1
ERROR: plugins/module_utils/api.py:224:1: E302: expected 2 blank lines, found 1

and also to use yield on a list in a different form for a file that is not touched by this pull request - only on Sanity Tests (devel) https://github.com/infobloxopen/infoblox-ansible/blob/master/tests/unit/compat/mock.py#L67 https://github.com/infobloxopen/infoblox-ansible/blob/master/tests/unit/compat/mock.py#L94

ERROR: Found 2 pylint issue(s) which need to be resolved:
ERROR: tests/unit/compat/mock.py:67:8: use-yield-from: Use 'yield from' directly instead of yielding each element one by one
ERROR: tests/unit/compat/mock.py:94:12: use-yield-from: Use 'yield from' directly instead of yielding each element one by one

On my branch of Matthew's PR where I added support for configuring multiple values on the extensible attributes via the flags (https://github.com/matthewdennett/infoblox-ansible/compare/master...hmalphettes:infoblox-ansible:create-ext-attrs-mattdennett) I added a commit to fix those linting issues.

I don't want to slow things down but if you want me to make a pull request or anything let me know!

JkhatriInfobox commented 3 months ago

Hi @matthewdennett We noticed a duplicate commit (fb83da309478d0fceacb50284d14b6b9cbefba0c) in your PR after last force push on your forked master. To maintain git history, please rebase your master and drop this commit, 

git rebase -i fb83da309478d0fceacb50284d14b6b9cbefba0c^

Replace “pick” with “drop” for the duplicate commit in the text editor that opens up.

Please note: This operation modifies the commit history, so please proceed with caution. If you’re unsure, consider creating a backup branch first or verify on local before pushing. Also, avoid force pushing after the rebase as it can disrupt the review process.

matthewdennett commented 3 months ago

Hi @JkhatriInfobox, that should be fixed up now. I've also included the changes @hmalphettes suggest.

Looks like the sanity tests are failing but the documentation looks correct to me. Have I missed something simple in the doco?

hmalphettes commented 3 months ago

@matthewdennett my thanks for putting in the support for flag.

The sanity tests point at some doc I wrote: ERROR: plugins/modules/nios_extensible_attribute.py:85:24: unparsable-with-libyaml: None - mapping values are not allowed in this context

Hopefully that explains the many more errors reported before that. I'll have a shot at fixing these later today.

hmalphettes commented 3 months ago

@matthewdennett these changes fixe the linting issues reported by the Sanity tests: https://github.com/hmalphettes/infoblox-ansible/commit/6af034df064fcd3f616a868d828ec469ea1093ea

matthewdennett commented 3 months ago

@hmalphettes awesome. I'll try and add those updates at some point tomorrow.

hmalphettes commented 3 months ago

@matthewdennett, we are left with one last line too long error. Short story: the failing workflow is running against an older version of nios_next_ip.py @JkhatriInfobox could you trigger a re-run? Otherwise we could always make yet another commit and push it to trigger that.

Here is the investigation if needed:

I checked out your code and ran the github actions from my github account: all is green.

I downloaded the artifacts attached to the latest failing github actions - https://github.com/infobloxopen/infoblox-ansible/actions/runs/9310510563/artifacts/1554330581 - and checked the failing nios_next_ip.py and it does not have the changes @matthewdennett kindly added to fix the issue.

image

When I do the same on my successful run - https://github.com/hmalphettes/infoblox-ansible/actions/runs/9314551322 - we find the expected version of nios_next_ip.py

I am hoping it is just a cache issue and all we need is to trigger the github actions again.

matthewdennett commented 3 months ago

I noticed this too and made a few commits but nothing seemed to make it register the change.

matthewdennett commented 3 months ago

@JkhatriInfobox, can you please have a look at the issue we are seeing here?

JkhatriInfobox commented 2 months ago

Hi @matthewdennett, @hmalphettes,

We appreciate your PR. Unfortunately, it didn't pass some checks. To address this, we've initiated a new PR with necessary corrections and will be closing PR #186 and #228 once new PR reviewed and merged . Your efforts are truly valued and we invite you to check out the new PR #235.

Additionally, we've identified some functional issues which we plan to rectify in the new PR. We also noticed the absence of unit and integration tests, which we'll be adding.

Thanks again for your contribution.

hmalphettes commented 2 months ago

@JkhatriInfobox thanks for the update and the work of the team on this. Happy to contribute in any manner.

JkhatriInfobox commented 2 months ago

Closing this Pull Request as the same modifications have been successfully merged in PR https://github.com/infobloxopen/infoblox-ansible/pull/235. This action is taken to maintain the cleanliness of the project and prevent redundancy. Please feel free to reach out if you need any further assistance.