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

fix: check for duplicate configs and secrets within a module #1328

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

deniseli
Copy link
Contributor

@deniseli deniseli commented Apr 24, 2024

Contributes to one part of #1121

@deniseli deniseli requested a review from a team as a code owner April 24, 2024 21:46
@deniseli deniseli requested review from matt2e and removed request for a team April 24, 2024 21:46
@alecthomas alecthomas mentioned this pull request Apr 24, 2024
@alecthomas
Copy link
Collaborator

The original issue was just about preventing duplicates in the schema, not necessarily in the code. I think this makes sense though, as it's a bit of a code smell to redeclare something in multiple locations.

for _, d := range pctx.module.Decls {
c, ok := d.(*schema.Config)
if ok && c.Name == name && c.Type.String() == st.String() {
pctx.errors.add(errorf(node, "duplicate config declaration"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's report the location of the other declaration in the message (here and below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea - done

@alecthomas
Copy link
Collaborator

You'll need to roll this out to the PFI repo, which will be ... interesting.

@deniseli
Copy link
Contributor Author

Notes to self (and anyone curious :) )

  • Add check for duplicate databases
  • Add checks to backend for all 3 types
  • Do not merge until after Matt and Wes's huge changes go in because we will want to strategize on how to roll this out to PFI, and that'll be simpler once their changes are done (or at least discussed/planned/strategized)

@alecthomas
Copy link
Collaborator

Don't delay merges, just get them in.

@deniseli
Copy link
Contributor Author

deniseli commented Apr 25, 2024

Got it, sounds good! I'll merge this to start (once everything passes) and get the rest of it done in a following PR. I'll move my TODOs to the issue for better tracking.

@deniseli deniseli merged commit 46c8ebe into main Apr 25, 2024
11 checks passed
@deniseli deniseli deleted the dli/schema-dupl-err branch April 25, 2024 01:47
@matt2e matt2e added the approved Marks an already closed PR as approved label Apr 25, 2024
deniseli added a commit that referenced this pull request Apr 26, 2024
Fixes #1121

This adds backend schema validation preventing duplicate configs,
secrets, and databases within a module. This also adds go-runtime
validation preventing duplicate databases. Go-runtime validation for
duplicate configs/secrets was added in PR
#1328
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Marks an already closed PR as approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants