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

Remove bascule #265

Open
wants to merge 7 commits into
base: feat/webhook-update
Choose a base branch
from
Open

Remove bascule #265

wants to merge 7 commits into from

Conversation

maurafortino
Copy link
Contributor

@maurafortino maurafortino commented Nov 11, 2024

Removed bascule from the transport.go file
TODO: update tests in transport_test file;
Discussion: not sure if the bascule replacement code is the most efficient

@maurafortino maurafortino added the needs discussion Further discussion is needed to move forward label Nov 11, 2024
@maurafortino maurafortino self-assigned this Nov 11, 2024
@maurafortino maurafortino marked this pull request as draft November 11, 2024 18:37
@maurafortino maurafortino changed the base branch from main to feat/webhook-update November 11, 2024 18:38
Copy link

guardrails bot commented Nov 11, 2024

All previously detected findings have been fixed. Good job! 👍🎉

We will keep this comment up-to-date as you go along and notify you of any security issues that we identify.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

@maurafortino maurafortino marked this pull request as ready for review November 14, 2024 17:06
return nil
}

func initializeKey() jwk.Key {
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep things consistent to our other repos, let's do the following 🙂

  • the string jwk should be made int a constant
  • have the tests themselves call jwk.ParseKey and verify the parser worked

Copy link
Contributor

@denopink denopink left a comment

Choose a reason for hiding this comment

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

Nice work! I only have 2 requested change, otherwise it lgtm! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Further discussion is needed to move forward
Projects
Status: PRs: Pending approval
Development

Successfully merging this pull request may close these issues.

2 participants