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

MLPAB-1638 Add update for when certificate provider signs #81

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

hawx
Copy link
Contributor

@hawx hawx commented Jan 3, 2024

No description provided.

@hawx hawx self-assigned this Jan 3, 2024
@hawx hawx requested a review from a team as a code owner January 3, 2024 08:29
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from cb93b36 to eefa788 Compare January 3, 2024 08:37
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from eefa788 to 3c89ca2 Compare January 3, 2024 09:04
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 3c89ca2 to 7ace6e8 Compare January 3, 2024 09:08
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 7ace6e8 to 16e3173 Compare January 3, 2024 09:11
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 16e3173 to 71145a4 Compare January 3, 2024 10:47
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 71145a4 to fde3a7d Compare January 3, 2024 10:57
@hawx hawx changed the title MLPAB-1638 Add lambda to create certificate provider MLPAB-1638 Add update for when certificate provider signs Jan 3, 2024
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from fde3a7d to e513d4f Compare January 3, 2024 12:54
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch 2 times, most recently from dbce62a to 05f20c8 Compare January 3, 2024 13:19
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 05f20c8 to 1b70f73 Compare January 3, 2024 13:24
})
}

_, err = pointer.Set(lpa, change.New)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would need to know the type it is going to set (if you want to see why the current one breaks, try setting /signedAt), so I've removed in favour of having a specific Apply object that does know what the types are

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for reworking that

@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 1b70f73 to bbbdf1a Compare January 3, 2024 14:18
@@ -9,6 +11,10 @@ type Address struct {
Country string `json:"country"`
}

func (a Address) IsSet() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is such a nit, but might be clearer to define IsZero() and check the inverse since "zero" already has clear meaning in Go (arguably person.Address = Address{} would be "set" even though it's empty).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, and it matches time.Time then which is nice

Comment on lines 26 to 28
verifier interface {
VerifyHeader(events.APIGatewayProxyRequest) bool
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the other lambdas this is still verifier shared.JWTVerifier, did this one need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because I wanted to inject a mock version in the tests. I'll name the type though to make it look nicer

})
}

_, err = pointer.Set(lpa, change.New)
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for reworking that

@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from bbbdf1a to 2bb18a3 Compare January 3, 2024 15:00
@hawx hawx force-pushed the MLPAB-1638-certificate-provider branch from 2bb18a3 to 9ce8afe Compare January 3, 2024 15:02
@hawx hawx merged commit 996b4b2 into main Jan 3, 2024
21 checks passed
@hawx hawx deleted the MLPAB-1638-certificate-provider branch January 3, 2024 15:22
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.

2 participants