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

use consumption to influence topology #777

Merged
merged 4 commits into from
Nov 21, 2023
Merged

use consumption to influence topology #777

merged 4 commits into from
Nov 21, 2023

Conversation

jhsinger-klotho
Copy link
Contributor

uses the consumption to influence topology yaml for this point in time

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?

if err != nil {
return val, err
}
val, err = TransformToPropertyValue(res, c.PropertyPath, val, ctx, DynamicValueData{Resource: res})
Copy link
Contributor

Choose a reason for hiding this comment

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

The double convert is very weird, why doesn't TransformtToPropertyValue do this? I think it would violate the contract of what that function should do if it doesn't return a fully valid, transformed value.

What's the use-case for why this won't work in one shot with the current implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we pass it a json string, it doesnt work in one shot. Thats what im running into now, because it converts the json string to map(string,string) but it doesnt evaluate the value to see if its a property ref or resource id. We can likely fix this function

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like it isn't doing the right thing in MapPropertyType.Parse. After decoding the string, it immediately returns instead of checking the sub properties.

@@ -545,3 +547,101 @@ func (r *ResourcePropertyType) ZeroValue() any {
func (p *PropertyRefPropertyType) ZeroValue() any {
return construct.PropertyRef{}
}

func (m *MapPropertyType) Contains(value any, contains any) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a subset than contains. Needs documentation too given that func(any, any) doesn't really give any signature-related hints to what should be passed in or valid types.

@jhsinger-klotho jhsinger-klotho merged commit 8e8ad14 into main Nov 21, 2023
79 of 85 checks passed
@jhsinger-klotho jhsinger-klotho deleted the consumption branch November 21, 2023 16:47
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