Skip to content

[-] fix duplicate source names in YAML configs, fixes #630#633

Merged
pashagolub merged 2 commits intocybertec-postgresql:masterfrom
amrdb:duplicate-yaml-sources-fix
Feb 5, 2025
Merged

[-] fix duplicate source names in YAML configs, fixes #630#633
pashagolub merged 2 commits intocybertec-postgresql:masterfrom
amrdb:duplicate-yaml-sources-fix

Conversation

@amrdb
Copy link
Contributor

@amrdb amrdb commented Feb 4, 2025

Fixes issue #630

(Hi, this is part of my preparation for GSoC 25)

@pashagolub pashagolub self-assigned this Feb 4, 2025
@pashagolub pashagolub added bug Something isn't working config Configuration store related labels Feb 4, 2025
@coveralls
Copy link

coveralls commented Feb 4, 2025

Pull Request Test Coverage Report for Build 13155279833

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 30.206%

Totals Coverage Status
Change from base Build 13111678393: 0.1%
Covered Lines: 1484
Relevant Lines: 4913

💛 - Coveralls

@pashagolub pashagolub changed the title [+] fix duplicate names in source file or source directory error handling, fixes #630 [-] fix duplicate source names in YAML configs, fixes #630 Feb 4, 2025
@pashagolub
Copy link
Collaborator

Thanks for your work! I decided to move duplicated functionality to the special routine Sources.Validadte(). That will give us the opportunity to use the same validation methods for all source readers.

Also I used a map[string]any{} instead of []string for uniqueness check. The idea behind is:

  1. map with empty struct holds less space than slice
  2. the random access is O(1) instead of O(n)

Otherwise the code looks solid and I especially like that you've added unit testing without further remind. :)

@pashagolub pashagolub merged commit 00693ba into cybertec-postgresql:master Feb 5, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working config Configuration store related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants