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

chore: expression for vars value #7411

Merged
merged 3 commits into from
May 24, 2024
Merged

chore: expression for vars value #7411

merged 3 commits into from
May 24, 2024

Conversation

leon-inf
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines. label May 23, 2024
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 65.47%. Comparing base (6c41811) to head (b958c44).
Report is 3 commits behind head on main.

Files Patch % Lines
pkg/controller/component/vars.go 94.73% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7411      +/-   ##
==========================================
+ Coverage   65.44%   65.47%   +0.02%     
==========================================
  Files         343      343              
  Lines       41699    41756      +57     
==========================================
+ Hits        27290    27339      +49     
- Misses      12022    12031       +9     
+ Partials     2387     2386       -1     
Flag Coverage Δ
unittests 65.47% <94.73%> (+0.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leon-inf leon-inf marked this pull request as ready for review May 23, 2024 05:59
@apecloud-bot apecloud-bot added the approved PR Approved Test label May 23, 2024
@apecloud-bot apecloud-bot removed the approved PR Approved Test label May 23, 2024
}
templateVars, envVars, err := ResolveTemplateNEnvVars(testCtx.Ctx, nil, synthesizedComp, vars)
Expect(err).Should(Succeed())
Expect(templateVars).Should(HaveKeyWithValue("endpoint", "localhost:12345"))
Copy link
Contributor

@weicao weicao May 23, 2024

Choose a reason for hiding this comment

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

seems to be an unexpected result, how about resolved for multiple passes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider such an example:

vars:
  - name: a
    value: 1
    expression: {{.a}}+{{.b}}
  - name: b
    value: 1
    expression: {{.a}}+{{.b}}

Even if we employ multiple passes to evaluate, it will never have a stable result.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can exclue the possibility of circular dependencies. If a circular dependency is detected, an error should be thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we do not parse and understand the content of the expressions (it's too complicated to do that), we have no idea about whether there are any circular dependencies, we can only check whether further iteration is needed by comparing the results of the two previous evaluations. As for whether the iteration can be ended or how many more times it‘s needed, there is no way for us to know it in advance. It may keep iterating indefinitely, unless we set an upper limit on the number of iterations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Set an upper limit on the number of iterations, throw an error if not converged. Sounds feasible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine, i will submit another PR to improve it.

@apecloud-bot apecloud-bot added the approved PR Approved Test label May 23, 2024
@leon-inf leon-inf merged commit 6e5ac26 into main May 24, 2024
63 checks passed
@leon-inf leon-inf deleted the support/vars-expression branch May 24, 2024 01:41
@leon-inf
Copy link
Contributor Author

/cherry-pick release-0.9

@github-actions github-actions bot added this to the Release 0.8.3 milestone May 24, 2024
github-actions bot pushed a commit that referenced this pull request May 24, 2024
Copy link

🤖 says: cherry pick action finished successfully 🎉!
See: https://github.com/apecloud/kubeblocks/actions/runs/9217348078

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved PR Approved Test size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants