-
Notifications
You must be signed in to change notification settings - Fork 39
feat(policy-devel): allow external policy references #2524
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
feat(policy-devel): allow external policy references #2524
Conversation
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
|
|
||
| func createPolicies(policyPath string, inputs map[string]string) (*v1.Policies, error) { | ||
| // Check if the policy path already has a scheme (chainloop://, http://, https://, file://) | ||
| ref := policyPath |
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.
check if there is smth already on the way we parse the policies in the contracts
| func (action *PolicyEval) Run() (*policydevel.EvalSummary, error) { | ||
| var attClient pb.AttestationServiceClient | ||
| if action.CPConnection != nil { | ||
| attClient = pb.NewAttestationServiceClient(action.CPConnection) |
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.
we might want to make sure this is not mandatory, I mean, if you do not provide a connection, the command should still work with file:// or http://
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.
It's not mandatory, we use it if it's available otherwise it works the same way as before and it will return jwt related error to the user for chainloop:// policies
migmartri
left a comment
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.
This is sweet, but is this a breaking change, in other words, would comments that used to provide a file path work out of the box?
| scheme, _ := policies.RefParts(policyPath) | ||
| if scheme == "" { | ||
| // Default to file:// | ||
| ref = fmt.Sprintf("file://%s", policyPath) | ||
| } |
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.
This is sweet, but is this a breaking change, in other words, would comments that used to provide a file path work out of the box?
Do you need to provide file:// always now?
It works out of the box, file:// is optional, backwards compatibility is held by this fragment and that's the only reason we have to check for scheme.
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.
awesome, that makes sense.
migmartri
left a comment
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.
See my suggestion, thanks!
app/cli/cmd/policy_develop_eval.go
Outdated
| cmd.Flags().StringVar(&kind, "kind", "", fmt.Sprintf("Kind of the material: %q", schemaapi.ListAvailableMaterialKind())) | ||
| cmd.Flags().StringSliceVar(&annotations, "annotation", []string{}, "Key-value pairs of material annotations (key=value)") | ||
| cmd.Flags().StringVarP(&policyPath, "policy", "p", "policy.yaml", "Path to custom policy file") | ||
| cmd.Flags().StringVarP(&policyPath, "policy", "p", "policy.yaml", "Policy reference (file://, http://, https://, chainloop://)") |
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'd personally remove file://, and add examples for example
| cmd.Flags().StringVarP(&policyPath, "policy", "p", "policy.yaml", "Policy reference (file://, http://, https://, chainloop://)") | |
| cmd.Flags().StringVarP(&policyPath, "policy", "p", "policy.yaml", "Policy reference (./my-policy.yaml, https://my-domain.com/my-policy.yaml, chainloop://my-stored-policy)") |
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
This PR adds support for external
http://, https://, chainloop://policy sources topolicy devel evalExamples: