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

update castle sdk remove deprecated fields #26

Merged
merged 9 commits into from
Nov 6, 2024

Conversation

pansbro12
Copy link

Updating the request sent to castle to include the created_at timestamp, context here, and fix general api drift.

@pansbro12 pansbro12 requested a review from a team as a code owner November 5, 2024 10:33
Copy link

linear bot commented Nov 5, 2024

client.go Outdated Show resolved Hide resolved
Copy link

@Jarota Jarota left a comment

Choose a reason for hiding this comment

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

Now that Risk and Filter aren't as similar as they were before, I wonder if it makes sense to split the code into a risk.go file and a filter.go file, where the relevant types are defined next to the corresponding API methods?

client.go Outdated Show resolved Hide resolved
model.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated
r := &castleAPIRequest{
params := Params{
Email: req.User.Email,
Username: req.User.Name,
Copy link

Choose a reason for hiding this comment

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

Is the name necessarily the username? I wonder if we're reusing the name field in our generic request type in a way that wasn't intended by the underlying API, or are they actually the same values?

Copy link
Author

@pansbro12 pansbro12 Nov 6, 2024

Choose a reason for hiding this comment

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

No so i actually was just thinking about this now, I remember us using either the account number or the email as username in a service somewhere but i can't think where.

Copy link
Author

Choose a reason for hiding this comment

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

Not really sure whether to include this now, looking at the code from the castle-processor the username is seems to be either the email or account number depending on what the user used to sign in. The username value is contained in the request properties so I've just set that as the username, might be that the username and the email are duplicates in some cases but I don't think that will be an issue.

@pansbro12 pansbro12 merged commit bbf4464 into master Nov 6, 2024
5 checks passed
@pansbro12 pansbro12 deleted the acc-944-update-request branch November 6, 2024 14:33
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