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

Prevent spurious diffs in config file by sorting Client objects before writing #3933

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

hnefatl
Copy link
Contributor

@hnefatl hnefatl commented Dec 12, 2021

This PR aims to prevent occasional reordering diffs like the following from appearing in the AdGuardHome.yaml config file:

 clients:
-- name: device 1
-  ids:
-  - x.x.x.x
-  ...
 - name: device 2
   ids:
   - y.y.y.y
   ...
+- name: device 1
+  ids:
+  - x.x.x.x
+  ...

This is problematic when including the config file in version control, since occasional no-op changes pop up and require reverting/committing.

I think the root cause of these reorders is that Go doesn't guarantee a consistent iteration order over maps (see the "Iteration order" section of https://go.dev/blog/maps), so e.g. different executions of an adguard docker container/daemon will produce different orders. By sorting the list of objects we create from the map before outputting them, we guarantee clients will be ordered by name.

I've only noticed this being an issue for the clients section of the config file: I haven't done a deep dive into the other config file fields, but from a quick skim of my config it looks they're probably not stored in maps so don't see this behaviour.

@ainar-g ainar-g self-assigned this Dec 13, 2021
@ainar-g
Copy link
Contributor

ainar-g commented Dec 13, 2021

Thanks for the contribution! We'll try to review it shortly.

@adguard adguard merged commit 7749377 into AdguardTeam:master Dec 13, 2021
@ainar-g ainar-g added this to the v0.107.0 milestone Dec 13, 2021
@ainar-g
Copy link
Contributor

ainar-g commented Dec 13, 2021

Looks great! We've made some additional adjustments and merged it. Thanks again for the contribution!

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

Successfully merging this pull request may close these issues.

3 participants