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.

Sign up for GitHub

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

Jump to bottom

chore: adr for store enhancement #933

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

chore: adr for store enhancement #933

wants to merge 6 commits into from

Conversation

cmwylie19
Copy link
Collaborator

@cmwylie19 cmwylie19 commented Jul 2, 2024 •
edited
Loading

Description

Extends existing Store functionality to v2 to enable storing keys like URLs, and provides a seamless hand-off migration path for users while introducing additional tooling to view the store resource.

  • How to enable the store to set special characters as keys
  • Address how to view the Store CR key/values in a user friendly way
  • Hands off end user migration path is decided on
  • Stakeholder feedback
  • UDS Core processes UDSExcemptions on startup so we want to leave that process unbothered (We think this should probably be fine but need to test. Other solutions pre-starup would involve initContainer). If this were a problem i think it could be solved by moving startExemptionWatch to the Store.onReady callback. IMO this checkbox is part of stakeholder feedback.

Also, i don't think this Store change will affect the startExemptionWatch function bc they are not using the actual Pepr Store in that instance but populating a map[string]interface{} type of store object.

Related Issue

Fixes #924

Relates to #918

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@cmwylie19 cmwylie19 changed the title chore: first pass at ADR for Store enhancement chore: ADR for Store enhancement Jul 2, 2024
@cmwylie19 cmwylie19 changed the title chore: ADR for Store enhancement chore: adr for Store enhancement Jul 2, 2024
@cmwylie19 cmwylie19 changed the title chore: adr for Store enhancement chore: adr for store enhancement Jul 2, 2024
btlghrants
btlghrants reviewed Jul 2, 2024
View reviewed changes
Copy link
Collaborator

@btlghrants btlghrants left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall, not a bad start -- and I've added some suggested refinements -- but... it reads more like a plan of action than an ADR, honestly -- it describes an issue and then just hops straight into a list of TODOs supporting a specific, apparently pre-decided implementation.

Where's the reasoning / alternatives? Will a future reader (or a soon-to-be reviewer) understand why we decided to solve the issue in the specific way we did?

For example:

  • Why did we decided to code this up instead of just dumping something into the docs saying "Here's the rules!"?
  • Why Base64 encode? Why not really sanitize the input by stripping out "bad" characters?
  • Why did you decide to put a "v2" in every key instead of having a v2 "container" injected in to keep the new / old keys separate (or whatever else)?
  • Why is it worth it to us to add a new Pepr command to the CLI when we could just tell user's it's base64'd and leave it up to them to decide how to read it?

...and so on. Happy to chat through any of that if you wanna, whenever.

adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
adr/00080-store-key-sanitization.md Show resolved Hide resolved
adr/00080-store-key-sanitization.md Outdated Show resolved Hide resolved
@cmwylie19
Copy link
Collaborator Author

Overall, not a bad start -- and I've added some suggested refinements -- but... it reads more like a plan of action than an ADR, honestly -- it describes an issue and then just hops straight into a list of TODOs supporting a specific, apparently pre-decided implementation.

Where's the reasoning / alternatives? Will a future reader (or a soon-to-be reviewer) understand why we decided to solve the issue in the specific way we did?

For example:

  • Why did we decided to code this up instead of just dumping something into the docs saying "Here's the rules!"?
  • Why Base64 encode? Why not really sanitize the input by stripping out "bad" characters?
  • Why did you decide to put a "v2" in every key instead of having a v2 "container" injected in to keep the new / old keys separate (or whatever else)?
  • Why is it worth it to us to add a new Pepr command to the CLI when we could just tell user's it's base64'd and leave it up to them to decide how to read it?

...and so on. Happy to chat through any of that if you wanna, whenever.

Sure, will add these fields. It does feel a little more like a KEP than an ADR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@btlghrants btlghrants btlghrants left review comments

@jeff-mccoy jeff-mccoy Awaiting requested review from jeff-mccoy jeff-mccoy is a code owner

@schaeferka schaeferka Awaiting requested review from schaeferka schaeferka is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Pepr Project Board
Status: 👀 In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store Enhancement Planning ADR
2 participants
@cmwylie19 @btlghrants