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

Process include/exclude from policy source #1026

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

lcarva
Copy link
Member

@lcarva lcarva commented Sep 19, 2023

@lcarva lcarva marked this pull request as draft September 19, 2023 17:12
go.mod Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #1026 (e291d15) into main (818fe8b) will increase coverage by 5.86%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1026      +/-   ##
==========================================
+ Coverage   79.33%   85.19%   +5.86%     
==========================================
  Files          58       66       +8     
  Lines        4969     5330     +361     
==========================================
+ Hits         3942     4541     +599     
+ Misses       1027      789     -238     
Flag Coverage Δ
acceptance 69.00% <85.71%> (?)
generative 52.34% <ø> (ø)
integration 62.59% <ø> (ø)
unit 71.21% <96.29%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage
...ation_snapshot_image/application_snapshot_image.go 100.00%
...nternal/evaluation_target/definition/definition.go 100.00%
internal/evaluator/conftest_evaluator.go 100.00%

@@ -199,13 +202,13 @@ func (r conftestRunner) Run(ctx context.Context, fileList []string) (result []Ou

// NewConftestEvaluator returns initialized conftestEvaluator implementing
// Evaluator interface
func NewConftestEvaluator(ctx context.Context, policySources []source.PolicySource, p policy.Policy) (Evaluator, error) {
return NewConftestEvaluatorWithNamespace(ctx, policySources, p, nil)
func NewConftestEvaluator(ctx context.Context, policySources []source.PolicySource, p policy.Policy, sc *ecc.SourceConfig) (Evaluator, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is ok for now, but I want us to reconsider this function signature. It seems redundant to pass in both the Policy and the PolicySource since the PolicySource is within the Policy. This is done, of course, so the evaluator knows which source group to use. I don't think the abstraction is quite right here.

We may want to consider this function only taking these parameters:

  • ctx context.Context obviously
  • src *ecc.Source. This should include the SourceConfig and enough information to build []source.PolicySource.
  • effectiveTime time.Time. This function doesn't need a whole Policy, just the effective time.

There's a wrinkle with this though. We still need something to combine the config from ecc.Source with the "global" config Policy.Spec().Configuration. That might be better off done prior to calling NewConftestEvaluator though which would make this a non-issue.

Anyways, this is somewhat subjective so I didn't want to include in this PR. If there's some agreement here, I will make a follow up PR to do so.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to refactor the signature here, perhaps something as simple as this could work?

// NewConftestEvaluator create a Evaluator implemented via Conftest to evaluate the n-th policy source
func NewConftestEvaluator(ctx context.Context, p policy.Policy, n int) (Evaluator, error) {

Copy link
Member

Choose a reason for hiding this comment

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

Or even:

// NewConftestEvaluators create a slice of Evaluator implementations via Conftest
func NewConftestEvaluators(ctx context.Context, p policy.Policy) ([]Evaluator, error) {

@lcarva lcarva marked this pull request as ready for review September 20, 2023 14:01
@lcarva lcarva merged commit 7a595ee into enterprise-contract:main Sep 20, 2023
@lcarva lcarva deleted the HACBS-2428 branch September 20, 2023 15:16
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