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

Bump Infoblox go client to v2 (2.1.2-0.20220727224704-88a5ba602dbf) #931

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jkremser
Copy link
Member

@jkremser jkremser commented Aug 2, 2022

Fix #920

The latest released version of this go client doesn't have the full
support for CRUDING the TXT records (get is missing) so we need to depend on a git sha.

It was a major version upgrade so some API was changed and some method signatures were modified.

Namely:

  • inicialization of the client is different, it now requires the auth config to be passed as a separate struct
  • creating TXT record requires a view argument
  • uint -> uint32 for ttl in method signatures
  • *ibcl.ObjectManager -> ibcl.IBObjectManager

Signed-off-by: Jirka Kremser [email protected]

@jkremser
Copy link
Member Author

jkremser commented Aug 2, 2022

I've also opened infobloxopen/infoblox-go-client#177 on the client's repo side so that we can later switch to properly released version.

The latest released version of this go client doesn't have the full
support for CRUDING the TXT records (get is missing) so we need to depend on a git sha.

It was a major version upgrade so some API was changed and some method signatures were modified.

Namely:
 - inicialization of the client is different, it now requires the auth config to be passed as a separate struct
 - creating TXT record requires a view argument
 - `uint -> uint32` for ttl in method signatures
 - `*ibcl.ObjectManager -> ibcl.IBObjectManager`

Signed-off-by: Jirka Kremser <[email protected]>
@jkremser jkremser marked this pull request as draft August 2, 2022 16:34
@jkremser jkremser changed the title Bump Infoblox go client to v2 (2.1.2-0.20220727224704-88a5ba602dbf) [wip] Bump Infoblox go client to v2 (2.1.2-0.20220727224704-88a5ba602dbf) Aug 2, 2022
@jkremser jkremser changed the title [wip] Bump Infoblox go client to v2 (2.1.2-0.20220727224704-88a5ba602dbf) Bump Infoblox go client to v2 (2.1.2-0.20220727224704-88a5ba602dbf) Aug 3, 2022
@jkremser jkremser marked this pull request as ready for review August 3, 2022 19:27
@somaritane
Copy link
Contributor

@jkremser It would be good to test the change with Infoblox instance.

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

LGTM assuming tested end-to-end

@somaritane
Copy link
Contributor

somaritane commented Sep 7, 2022

LGTM assuming tested end-to-end

@ytsarev it's not, also it pins non officially released version.
Taking this and kubernetes-sigs/external-dns#2945 (comment) into account,
I suggest we park the PR until we sort all the points out, as discussed in project meeting.

@somaritane somaritane marked this pull request as draft September 7, 2022 11:20
@jkremser
Copy link
Member Author

jkremser commented Sep 7, 2022

agreed on parking

@jkremser It would be good to test the change with Infoblox instance.

#920 (comment)

Yep, for changes like this the mock tests that we currently have are pretty much useless. They changed the contract of some methods and I had to change the mocking, because they were making assumptions about internal implementations of the API.. like how many times some internal method is being called etc. It would be much more useful to have a way to spawn infoblox service and test it against it. However, if I get it right, there is no easy way to do that. I wasn't able to find any public container image for that.. In the official docs, they suggest to download the tar from their web ui and "docker load" it - wow. I am not a lawyer, but I think this altered image won't be freely usable in the public CI on GitHub. I can also check if the client library has some real tests against Infoblox.

UPDATE: the infoblox-client also doesnt' have tests against the real instance - https://github.com/infobloxopen/infoblox-go-client/blob/master/object_manager_a-record_test.go#L31:L43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infoblox provider: upgrade infoblox client to recent version
3 participants