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

Add Explicit Commit, Dynamo Equivalence Check, and Temporary and Permanent Error Distinctions #179

Merged
merged 100 commits into from
Nov 16, 2022
Merged

Add Explicit Commit, Dynamo Equivalence Check, and Temporary and Permanent Error Distinctions #179

merged 100 commits into from
Nov 16, 2022

Conversation

Sneagan
Copy link
Collaborator

@Sneagan Sneagan commented Aug 4, 2022 •
edited
Loading

No description provided.

@Sneagan Sneagan force-pushed the feature/temporary-permanent-error-distinctions branch from aeee36c to 2368408 Compare August 11, 2022 19:10
@Sneagan Sneagan requested review from evq, clD11 and husobee August 11, 2022 19:34
@Sneagan Sneagan marked this pull request as ready for review August 11, 2022 19:34
@Sneagan Sneagan changed the title Feature/temporary permanent error distinctions Add Temporary and Permanent Error Distinctions Aug 11, 2022
@Sneagan Sneagan mentioned this pull request Nov 11, 2022
Open
@Sneagan Sneagan requested review from evq and husobee November 14, 2022 14:48
@husobee husobee force-pushed the feature/temporary-permanent-error-distinctions branch from 3aee845 to 1329db6 Compare November 14, 2022 17:14
@husobee
Move linting step into CI github workflow.
0c9f202 
Linting corrections, test corrections.
@husobee husobee force-pushed the feature/temporary-permanent-error-distinctions branch from 0796108 to 0c9f202 Compare November 15, 2022 00:35
husobee
husobee approved these changes Nov 15, 2022
View reviewed changes
Copy link

@husobee husobee left a comment

Choose a reason for hiding this comment

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

Walking through the batch pipeline processing, it might be a good call to have some tests that fully excercise a working environment to prove out that there will not be any issues.

server/dynamo.go Outdated Show resolved Hide resolved
utils/errors.go Outdated Show resolved Hide resolved
kafka/main.go Outdated Show resolved Hide resolved
husobee
husobee suggested changes Nov 15, 2022
View reviewed changes
Copy link

@husobee husobee left a comment

Choose a reason for hiding this comment

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

actually, changed my mind. I would like to see this working in a full test. I think using interfaces would make it easier to test, like an interface for a processor.

Since this is a big change, and we are about to deploy to production the kafka implementation for tlv2, I think it would be best to wait to merge this until after we merge the issuer rotation fix and get that deployed. my 2c

clD11
clD11 reviewed Nov 15, 2022
View reviewed changes
kafka/signed_blinded_token_issuer_handler.go Show resolved Hide resolved
kafka/signed_blinded_token_issuer_handler.go Show resolved Hide resolved
kafka/main.go Show resolved Hide resolved
@Sneagan
Make batchPipeline a buffered channel
f4ca678
@Sneagan
Merge branch 'feature/temporary-permanent-error-distinctions' of gith…
617c4f4 
…ub.com:brave-intl/challenge-bypass-server into feature/temporary-permanent-error-distinctions
clD11
clD11 reviewed Nov 15, 2022
View reviewed changes
kafka/main.go Show resolved Hide resolved
@Sneagan
Copy link
Collaborator Author

Sneagan commented Nov 15, 2022

@husobee What do you have in mind for a full test? I can implement unit tests for a lot of this behavior, but the kafka integration really needs to run with a proper kafka cluster and that's complicated to get into a test.

@husobee
Copy link

husobee commented Nov 16, 2022

@husobee What do you have in mind for a full test? I can implement unit tests for a lot of this behavior, but the kafka integration really needs to run with a proper kafka cluster and that's complicated to get into a test.

we can use the skus-sdk end to end testing functionality to test the signing flow. will setup a test for this.

husobee
husobee previously approved these changes Nov 16, 2022
View reviewed changes
@husobee husobee merged commit 5726939 into master Nov 16, 2022
@husobee husobee deleted the feature/temporary-permanent-error-distinctions branch November 16, 2022 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clD11 clD11 clD11 left review comments

@husobee husobee husobee approved these changes

@johnhalbert johnhalbert Awaiting requested review from johnhalbert

@evq evq Awaiting requested review from evq

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Sneagan @husobee @johnhalbert @evq @clD11