Skip to content

Conversation

nikpivkin
Copy link
Contributor

@nikpivkin nikpivkin commented Apr 4, 2025

Description

See issue description

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
@nikpivkin nikpivkin marked this pull request as ready for review April 7, 2025 09:29
@nikpivkin nikpivkin requested a review from simar7 as a code owner April 7, 2025 09:29
Comment on lines 342 to 346
p.logger.Warn(
"Variable values was not found in the environment or variable files. Evaluating may not work correctly.",
log.String("variables", strings.Join(missingVars, ", ")),
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
p.logger.Warn(
"Variable values was not found in the environment or variable files. Evaluating may not work correctly.",
log.String("variables", strings.Join(missingVars, ", ")),
)
}
p.logger.Warn(
"Variable values were not found in the environment or variable files. Evaluating may not work correctly.",
log.String("variables", strings.Join(missingVars, ", ")),
)
}

if !ctyVal.IsKnown() || ctyVal.IsNull() {
if ctyVal.IsNull() {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit to using .IsWhollyKnown() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think not, since IsWhollyKnown checks that complex types are fully known, such as lists or objects, which would prevent us from using only some elements of those types if needed. Also, I don't see the benefit of returning DynamicVal when the value is unknown, as it results in type erasure.

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ on this change 👍

Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
@simar7 simar7 added this pull request to the merge queue Apr 8, 2025
Merged via the queue into aquasecurity:main with commit 9dcd06f Apr 8, 2025
12 checks passed
@aqua-bot aqua-bot mentioned this pull request Apr 8, 2025
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.

fix(misconf): add missing variable as unknown
3 participants