Skip to content

Conversation

@mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Aug 8, 2016

This is the API most applications should use to get the policy for the current host.

Also adds a types.SystemContext per discussions in #41 and elsewhere, to make the functions testable and usable in special situations like chroots.

(Though, signature.DefaultPolicy() with an override is not that different from signature.NewPolicyFromFile().)

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 8, 2016

(I have also considered using RootForImplicitAbsolutePaths in the SystemContext, as a more general way to override many paths at once; e.g. unprivileged users could set this to ~/.atomic-root and then create ~/.atomic-root/etc/atomic/policy.json and the like.

At the moment, a simple explicit override for that one path is easier to use; we may consider the RootForImplicitAbsolutePaths again in the future.)

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 10, 2016

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 10, 2016

@runcom
Copy link
Member

runcom commented Aug 25, 2016

lgtm

Approved with PullApprove

if err != nil {
policy = &Policy{Default: PolicyRequirements{NewPRReject()}}
}
return policy, err
Copy link
Member

Choose a reason for hiding this comment

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

return policy, nil? (or am I missing something?)

Copy link
Member

Choose a reason for hiding this comment

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

alright, got it but it's pretty confusing.

User of this function at https://github.com/projectatomic/skopeo/pull/157/files#diff-c6c318da062a6b9637745a480d10434eR88 bail out on error so why setting in line 56 a default policy if we're stil going to return the error here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying it's wrong, perhaps could be rewritten to be more obvious?

@runcom
Copy link
Member

runcom commented Aug 25, 2016

just nitpicking after reviewing related PRs

This is the API most applications should use to get the policy for the
current host.

Also adds a types.SystemContext per discussions in
containers#41 and elsewhere, to make the
functions testable and usable in special situations like chroots.

(Though, signature.DefaultPolicy() with an override is not that
different from signature.NewPolicyFromFile().)

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 25, 2016

Updated to move the compiled-in path to /etc/containers/policy.json, and to return nil policy on error instead of returning an internal fallback.

@runcom
Copy link
Member

runcom commented Aug 25, 2016

LGTM

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 25, 2016

👍

Approved with PullApprove

@mtrmac mtrmac merged commit 584108d into containers:master Aug 25, 2016
@mtrmac mtrmac deleted the default-policy branch August 25, 2016 17:21
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