-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add modules to manipulate additional types of Name Server Groups #56
Add modules to manipulate additional types of Name Server Groups #56
Conversation
Hi @badnetmask, |
Will be glad to review my changes as soon as I can find some available cycles. |
WIP -- I have fixed the merge conflicts, but I believe the new standards require integration tests, so I will work on these later. |
Thank you! Yes, according to the new standards there's a requirement for unit and integration tests. I've suggested a few changes to fix the sanity error. |
…al system, wanna check what happens on github)
@anagha-infoblox I need help figuring out how to create the tests. For whatever reason, when I run a playbook that uses all these new objects, against a real Infoblox device, all the objects are created just fine. However, running the collection in integration tests mode, they don't due to an error on the "infoblox-client" library. You can see the error on here on GitHub, on the checks tab. So I'm not sure what's happening here, since the integration tests docker image uses the same version of the library that I have installed on my system (0.5.0). Any help here would be appreciated. |
Here's a quick example to demonstrate how it's working on a real Infoblox device:
|
@badnetmask I see that the tests are failing because of the NoneType error. Before the tests were failing due to the error stated above for stable-2.12 and devel branches. It was fixed by adding this file under integration tests- https://github.com/infobloxopen/infoblox-ansible/blob/master/tests/integration/targets/prepare_nios_tests/tasks/prepare_nios_tests_idempotence.yml
under the tasks. |
Well, I basically copied the entire tree under the |
@badnetmask Sorry for the delayed reply. The integration tests are failing because these objects are not added to the nios-simulator. The tests are being run with the help of a simulator that mimics the functionality of a NIOS grid. We will look into the procedure of adding new objects to the simulator. |
- install package 'cargo' on the image (required to build 'cryptography') - update flaskapp.py to add support for additional nsgroup:delegation (required for infobloxopen/infoblox-ansible#56)
NOTE: requires update on 'nios-test-container' which has not been pushed yet
NOTE: automated github tests in this repo will fail until this is merged, and a new image is pushed to quay.io: ansible/nios-test-container#8
Container updates: - update image to alpine 3.15 - install cryptography using apk Updates to support infobloxopen/infoblox-ansible#56: - update flaskapp.py to add support for additional nsgroup:delegation - add more nsgroup objects
@anagha-infoblox - good and bad news. |
@badnetmask The container used can be overridden by setting the |
@anagha-infoblox I suppose the container override needs to be configured on the repo level, which I do not have access. Please, let me know your thoughts. |
@badnetmask I have configured the |
Seems like the integration tests are not picking up the modified variable. |
Yes. I will try this again and, the modules will be included in the next major release. |
@badnetmask, I have configured the |
@anagha-infoblox looks like all checks passed now \o/ |
Thank you for your contribution to the Infoblox NIOS modules. According to the new standards, there is a requirement for unit and integration tests. The PR will be merged once apt unit tests for the modules are added. |
I'm having an issue here while trying to run the unit test locally (trying to avoid spamming GitHub checks). This command:
Fails with this error:
What I don't understand is how these tests run just fine from GitHub if this module does not exist in the repo. |
@badnetmask, Sorry for the delayed reply. The CI workflow is failing because of the same issue. Will make the required changes soon. |
@badnetmask, I have raised a Pull Request with the changes required to pass the unit tests. Once the unit tests are added, the PR: |
The Pull Request is merged. With the latest code
The Pull Request is merged. With the latest code, the above issue is fixed. Sorry for the delayed response. |
Seems like I still have issues running the unittests on my end, but I'll spend some time figuring it out before complaining. 😄 |
@badnetmask, can you please push the unit tests that you have written? We can verify the same if any issues persist. |
@JchhatbarInfoblox @JkhatriInfobox - this PR has been pending for a few years, and ended up forgotten because I didn't fix some pending issues. Now that they are fixed, would you mind giving it a review? |
@badnetmask - We’ve initiated the review process for your Pull Request. It appears that only Integration tests have been included, with Unit tests missing. Could you please commit the Unit test cases to the PR? This will ensure it passes both the Unit and Integration tests in the CI run. Additionally, the modifications made to the file [tests/unit/compat/mock.py] have already been merged into the master branch. To maintain a clean commit history, please rebase your branch with the master. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The addition of new objects with respect to nsgroup looks good.
- Unable to verify the Continuous Integration (CI) for the PR as it is quite old.
- Unit test cases are not included in the PR and need to be addressed by the internal development team.
- Integration tests need to be monitored for any failures after the merge.
This PR adds support for additional types of Name Server Groups.
The original nsgroup module only handles Authoritative type. This PR also modifies the description of the original
nsgroup
module to reflect that.Each module's documentation contain examples.