search

Kong / go-database-reconciler

Apache License 2.0
5 stars 3 forks source link

chore: update go-kong to v0.57.1 #122

Closed Prashansa-K closed 2 months ago

Prashansa-K commented 2 months ago

The new go-kong version ensures that shorthand_fields are filled with defaults or nested config values. If the shorthand_fields are set, the nested fields are backfilled to keep both in sync. This ensures that deck diff or deck sync commands do not show unnecessary updates in plugin configs.

Fix for #1251 from deck issues.

pmalek commented 2 months ago

It looks like test failures needs addressing https://github.com/Kong/go-database-reconciler/actions/runs/10252010211/job/28361391166?pr=122

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 30.04%. Comparing base (8de497a) to head (de740f2). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #122 +/- ## =========================================== - Coverage 42.72% 30.04% -12.68% =========================================== Files 75 106 +31 Lines 8819 12547 +3728 =========================================== + Hits 3768 3770 +2 - Misses 4587 8314 +3727 + Partials 464 463 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Prashansa-K commented 2 months ago

It looks like test failures needs addressing https://github.com/Kong/go-database-reconciler/actions/runs/10252010211/job/28361391166?pr=122

Fixed the test. rbac_role_get test was erroring out. Main issue was a missing name field due to which rbac_role wasn't getting added. Not sure how it was passing earlier. We had a //nolint gosec comment beside the test runner. Could this be causing a skip?

There's a rbac_role add test just above the changed test that shows that without a name, any addition would error out. https://github.com/Kong/go-database-reconciler/blob/main/pkg/state/rbac_role_test.go#L36

cc: @GGabriele

pmalek commented 2 months ago

We had a //nolint gosec comment beside the test runner. Could this be causing a skip?

That should only affect the linter, not whether the tests were skipped 🤔

The actual issue that this nolint was trying to cover up was https://go.dev/blog/loopvar-preview, which got "fixed" in 1.22. So this directive is not needed anymore.

Prashansa-K commented 2 months ago

This PR was closed as some more changes may be needed in go-kong for properly addressing the issue of deprecated schema fields logic and ensuring that gateway 3.7.x and 3.8.x related changes don't conflict. More info on this slack thread: https://kongstrong.slack.com/archives/C07AQH7SAF8/p1722589141558609