-
Notifications
You must be signed in to change notification settings - Fork 51
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
refactor: Change local_acp implementation to use acp_core #2691
Conversation
todo: Please sort out the PR description :) Adding info regarding why this is being done would be/have-been particularly useful. And please make sure it is linked to a Defra repo github issue. |
Is there any action required on my part regarding the |
It looks like the vulnerability only exists in the runtime/toolchain version you tried to update to - so if you sort out the go.mod that job should pass anyway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't sure If I should review the PR or not as I wasn't pinged again for review since last review, but it seemed like there was some activity and you did some suggestions from the previous review, so leaving some small todos for ya, it looks really good now :)
acp/acp_local.go
Outdated
marshalType := types.PolicyMarshalingType_SHORT_YAML | ||
if isJSON := fastjson.Validate(policy) == nil; isJSON { // Detect JSON format. | ||
marshalType = types.PolicyMarshalingType_SHORT_JSON | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why was the detection moved from source_hub_client
into the local acp
logic? Wouldn't this be nicer for @AndrewSisley's remote acp implementation as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, fair. I'll add a custom Defra type for it, as I did for the Registration Result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just leaving as it was before should be fine? why do we need a custom type?
EDIT: I see what you mean, just reviewed the last commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left one question, please resolve before merging. Giving an LGTM.
Thanks @Lodek for doing all the requested changes :)
WOOT WOOT your 1st DefraDB PR 🥳
Also the related issue won't close upon merge, nor is it picked up by github at the moment, because you just have So please don't forget to add the resolving key word ( So you can put for example:
|
Relevant issue(s)
Resolve #2694
Description
This PR changes the underlying implementation of the
ACPLocal
Reference Monitor from SourceHub to ACP Core.The changes aim to allow the WASM build which was broken since the inclusion of SourceHub as a dependency.
Notable changes from this refactor include:
name
in aPolicy
is now correctly marked as required. This was a consequence of an issue on SourceHub side which allowed unnamed policies to be included. Therefore, in order to correct the tests, a name was given to all policies in the codebase.Identity
struct to generate a DID instead of a SourceHub addressTasks
How has this been tested?
Test suite ie
make test
Specify the platform(s) on which this was tested: