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

Fail On Duplicate Config Names #1372

Merged
merged 8 commits into from
Jan 8, 2025

Conversation

paulgibert
Copy link
Contributor

Fixes #1599

  • Updated wolfictl lint to fail if a duplicate package config name is encountered.
  • Added a unit test to melange_test.go for duplicate package config names

Steps to test:

  1. See that lint succeeds on a valid repo:
git clone https://github.com/wolfi-dev/os.git
cd os
wolfictl lint -s error

# Should succeed
  1. See that introducing duplicate configs results in an error:
# From inside os/

echo "package:\n  name: duplicate\n  version: 1.2.3" > duplicate1.yaml
cp duplicate1.yaml duplicate2.yaml
wolfictl lint -s error

# Example output: 2024/12/13 11:44:33 ERRO Package config names must be unique. Found duplicate 'duplicate'

The check happens in pkg/melange as this is where the duplicates were ignored as pointed out in #1599. An alternative but more complex solution would be to add a new lint rule. I didn't go that route because it would require a notable ABI change.

#first-golang-pr (let's go)

@priyawadhwa

@paulgibert paulgibert changed the title Block Duplicate Config Names Fail On Duplicate Config Names Dec 13, 2024
@@ -0,0 +1,3 @@
package:
name: foo
Copy link
Member

Choose a reason for hiding this comment

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

I think we might also want to fail if .package.name is not the same as the filename.

These are potentially different/separate issues, but it does I think violate some assumptions we've made, and is worth avoiding.

If we have that check, we also get uniqueness "for free" since there can't be two foo.yamls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the .package.name field must match the filename, couldn't you make an argument that the field is redundant and should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check to error if the config name does not match the package name. This makes it impossible to test the package duplicate logic because, as you said, this gives us uniqueness for free, but I left the original check in just in case.

Copy link
Member

Choose a reason for hiding this comment

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

It probably is redundant, but I don't think we can easily remove it without causing lots and lots of downstream effects. I think it's fine to leave it, and just ensure it's the same as the filename, for now.

imjasonh
imjasonh previously approved these changes Dec 13, 2024
89luca89
89luca89 previously approved these changes Dec 13, 2024
pkg/melange/melange.go Outdated Show resolved Hide resolved
@paulgibert paulgibert dismissed stale reviews from 89luca89 and imjasonh via c797060 January 7, 2025 21:28
imjasonh
imjasonh previously approved these changes Jan 8, 2025
priyawadhwa
priyawadhwa previously approved these changes Jan 8, 2025
@priyawadhwa
Copy link
Member

Looks like there are some linting errors for extra whitespace in the file, just need to clean that up and this should be ready to merge!

@paulgibert paulgibert dismissed stale reviews from priyawadhwa and imjasonh via 2a71d32 January 8, 2025 17:38
@priyawadhwa priyawadhwa merged commit b478afe into wolfi-dev:main Jan 8, 2025
4 checks passed
@priyawadhwa
Copy link
Member

Nice!

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.

4 participants