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

Some cool change #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Some cool change #1

wants to merge 11 commits into from

Conversation

jkremser
Copy link
Owner

No description provided.

@netlify
Copy link

netlify bot commented Oct 14, 2021

✔️ Deploy Preview for k8gb-preview ready!

🔨 Explore the source changes: 109d141

🔍 Inspect the deploy log: https://app.netlify.com/sites/k8gb-preview/deploys/61686099965a7100860573f4

😎 Browse the preview: https://deploy-preview-1--k8gb-preview.netlify.app/

@jkremser jkremser closed this Oct 14, 2021
@jkremser jkremser reopened this Oct 14, 2021
@jkremser jkremser closed this Oct 14, 2021
@jkremser jkremser reopened this Oct 14, 2021
@ytsarev
Copy link

ytsarev commented Oct 15, 2021

Really awesome 👍

@jkremser jkremser force-pushed the master branch 13 times, most recently from fb4dddb to 77c3008 Compare October 21, 2021 14:59
@jkremser jkremser force-pushed the master branch 4 times, most recently from 9856337 to c35fa28 Compare October 26, 2021 08:50
jkremser pushed a commit that referenced this pull request Nov 1, 2021
related to k8gb-io#697

 `k8s.io/*` bundle to `v0.22.2` (controller-gen tool makes changes in CRD description after bump)
 - `github.com/golang/mock` to `v1.6.0` (is not compiled into final binary but checked by dependabot)
 - `github.com/miekg/dns` to `v1.1.43`
 - `github.com/infobloxopen/infoblox-go-client` to `v1.1.1` (changes argument CreateTXTRecord from integer to uint)
 - `sigs.k8s.io/controller-runtime` to `v0.10.2`, had to fix the test, see comment below.
 - `sigs.k8s.io/external-dns` is problematic in terms it uses latest `github.com/go-logr/logr` which is incompatible
with previous versions. Will be part of followup PR

Controller-runtime works on copies of annotations rather than their pointers, so I had to modify the test.

```go
func TestGslbProperlyPropagatesAnnotationDownToIngress(t *testing.T) {
        // arrange
        settings := provideSettings(t, predefinedConfig)
        expectedAnnotations := map[string]string{"annotation": "test"}
        settings.gslb.Annotations = expectedAnnotations
        err := settings.client.Update(context.TODO(), settings.gslb)
        require.NoError(t, err, "Can't update gslb")
        // act
        reconcileAndUpdateGslb(t, settings)
        err2 := settings.client.Get(context.TODO(), settings.request.NamespacedName, settings.ingress)
        // assert
        assert.NoError(t, err2, "Failed to get expected ingress")
        assert.Equal(t, expectedAnnotations, settings.ingress.Annotations)
}
```

If I extend fial assertion the passing test is doing this:
```go
assert.Equal(t, map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}, settings.ingress.Annotations)
assert.Equal(t, map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}, settings.gslb.Annotations)
assert.Equal(t, map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}, expectedAnnotations)
```

The `"k8gb.io/strategy": "roundRobin"` is added to `expectedAnnotations` during `reconcileAndUpdateGslb` and controlle-runtime
is the guy which extends `expectedAnnotations`.

I bumped controller runtime to `v0.10.2`, and only this concrete test has to be updated, because expectedAnnotations are not altered within controller-runtime:
```go
assert.Equal(t, map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}, settings.ingress.Annotations)
assert.Equal(t, map[string]string{"annotation": "test", "k8gb.io/strategy": "roundRobin"}, settings.gslb.Annotations)
assert.Equal(t, map[string]string{"annotation": "test"}, expectedAnnotations)
```

Signed-off-by: kuritka <[email protected]>
@jkremser jkremser force-pushed the master branch 3 times, most recently from def7bc8 to 2a47add Compare November 22, 2021 12:10
@jkremser jkremser force-pushed the master branch 3 times, most recently from 07639c6 to 664110a Compare December 13, 2021 21:03
@jkremser jkremser force-pushed the master branch 4 times, most recently from 3cdda35 to 59eec76 Compare December 22, 2023 00:54
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.

2 participants