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

Print errors to stdout #920

Merged
merged 8 commits into from
Feb 8, 2024
Merged

Print errors to stdout #920

merged 8 commits into from
Feb 8, 2024

Conversation

gordon-klotho
Copy link
Contributor

Note: the added testdata cases showing the validation errors are demonstrating some gaps in the validation logic, but were unchanged by this PR.

Standard checks

  • Unit tests: Any special considerations?
  • Docs: Do we need to update any docs, internal or public?
  • Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working?

{
"error": {
"chain": [
"required property Id is not set on resource aws:subnet:vpc-0:subnet-0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Id is one of the "required + deploy time" variables. Those should probably not cause validation errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we have a card for this, so we might want to pick it up as a part of this workstream

"children": [
{
"chain": [
"invalid int value 80"
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 this is because they're "80" (string) and not being converted to an int

Copy link
Contributor

Choose a reason for hiding this comment

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

that should be able to be converted but if not we need to either cut a card or update the code

@gordon-klotho
Copy link
Contributor Author

Don't know why the unit tests fail, they've been consistently successful for me. Looks like maybe some non-determinism on op eval?

@jhsinger-klotho
Copy link
Contributor

"showing the validation errors are demonstrating some gaps in the validation logic, but were unchanged by this PR." are the gaps just the 2 things you commented or are there more?

pkg/engine2/cli.go Outdated Show resolved Hide resolved
pkg/engine2/cli.go Outdated Show resolved Hide resolved
pkg/engine2/cli.go Show resolved Hide resolved
pkg/engine2/solution_context/decisions.go Outdated Show resolved Hide resolved
@gordon-klotho gordon-klotho force-pushed the feature/stdout_details branch 2 times, most recently from 89a6187 to 07b9c19 Compare February 1, 2024 15:16
@gordon-klotho gordon-klotho force-pushed the feature/stdout_details branch from d258674 to 15af974 Compare February 7, 2024 17:19
@gordon-klotho gordon-klotho merged commit 8cf5a3a into main Feb 8, 2024
6 checks passed
@gordon-klotho gordon-klotho deleted the feature/stdout_details branch February 8, 2024 17:36
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