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.

Sign up for GitHub

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

Jump to bottom

Feat: Addon support app template written by cuelang. #4401

Merged
merged 6 commits into from Jul 22, 2022
Merged

Feat: Addon support app template written by cuelang. #4401

merged 6 commits into from Jul 22, 2022

Conversation

wangyikewxgm
Copy link
Collaborator

@wangyikewxgm wangyikewxgm commented Jul 18, 2022 •
edited

This pr support writing basic addon application template framework by cue, so addon-developer can define the final app with parameter.Will allow user define a parameter.cue file outside resource dir. All parameters defined in this file will affect resources in resources dir and template.cue.

So a new addon dir is like this:

definitions/
resources/
——component1.cue
——component2.yaml
template.cue
parameter.cue
metadata.yaml

Besides, this pr also dose such things:

  1. Keep compatibility for old addon, new version CLI can still enable old version addon.
  2. If an addon defines deployTo.runtimeCluster=true want to deploy to runtime clusters but still don't have a topology polocy, we assume it's a legacy addon, will attach a topology policy to this app, not two workflowSteps(deploy-local, deploy-all-runtime) as before.
  3. Delete all hack code for observability addon, because all these codes only for selecting cluster and override policy for different cluster, this will handled by writing a template.cue.
  4. Refactor some codes, put render related code to a single file.

Description of your changes

Fixes #4283

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Special notes for your reviewer

@wangyikewxgm wangyikewxgm marked this pull request as draft July 18, 2022 06:15
@Somefive Somefive changed the title [Feat]: Addon support app template written by cuelang. Feat: Addon support app template written by cuelang. Jul 18, 2022
@codecov
Copy link

codecov bot commented Jul 18, 2022 •
edited

Codecov Report

Merging #4401 (67f3f27) into master (67f3f27) will not change coverage.
The diff coverage is n/a.

❗ Current head 67f3f27 differs from pull request most recent head 95bedda. Consider uploading reports for the commit 95bedda to get more accurate results

@@           Coverage Diff           @@
##           master    #4401   +/-   ##
=======================================
  Coverage   59.92%   59.92%           
=======================================
  Files         347      347           
  Lines       34281    34281           
=======================================
  Hits        20544    20544           
  Misses      10986    10986           
  Partials     2751     2751
Flag Coverage Δ
apiserver-e2etests 27.09% <0.00%> (ø)
apiserver-unittests 40.08% <0.00%> (ø)
core-unittests 56.48% <0.00%> (ø)
e2e-multicluster-test 19.79% <0.00%> (ø)
e2e-rollout-tests 22.19% <0.00%> (ø)

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

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67f3f27...95bedda. Read the comment docs.

@Somefive Somefive mentioned this pull request Jul 19, 2022
Open
@wangyikewxgm wangyikewxgm force-pushed the addon-support-cue-template branch 5 times, most recently from 1204157 to a33bd6e Compare July 20, 2022 07:28
@FogDong FogDong mentioned this pull request Jul 20, 2022
Merged
5 tasks
@wangyikewxgm wangyikewxgm force-pushed the addon-support-cue-template branch 3 times, most recently from 6ac41c0 to 9fc3f56 Compare July 20, 2022 11:53
@wangyikewxgm wangyikewxgm marked this pull request as ready for review July 21, 2022 02:32
wonderflow
wonderflow reviewed Jul 21, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
charlie0129
charlie0129 reviewed Jul 21, 2022
View reviewed changes
pkg/addon/render.go

app.Name = addonutil.Addon2AppName(addon.Name)
// force override the namespace defined vela with DefaultVelaNS,this value can be modified by Env
app.SetNamespace(types.DefaultKubeVelaNS)
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering how can this value be modified with env?

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we support vela addon enable --namespace x now ? It's a TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is by design. All addon associated application in same name is more convenient to manage. But user still can set DEFAULT_VELA_NS env to change this.

wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go

// This func can be used for addon render, supporting render app template and component.
// Please notice the result will be stored in object parameter, so object must be a pointer type
func (a addonCueTemplateRender) toObject(cueTemplate string, object interface{}) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func (a addonCueTemplateRender) toObject(cueTemplate string, object interface{}) error {
func (a addonCueTemplateRender) render(cueTemplate string) (*Application,error) {

This function can only output Application cause you have specific logic inside.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This func can be called by both render app and appCompnent, that's determined by input object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if so ,you should make the cuepath output as an input, it can become much more common function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why we need change this path? The appComponent and app defined in cue file are all in output block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how would you deal with the case for "outputs"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the input parameter is cueTemplate file which contain one output and several outputs, and the outputs is unstructed object, I plan to add a input parameter value as aliaOutputs []unstructed.unstructure which fetch all outpus object in this cueTemplate

wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go
if err != nil {
return err
}
contextFile.WriteString(fmt.Sprintf("context: metadata: %s\n", string(metadataJSON)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

will the metadataJSON be empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, empty meta will get {"name":"","version":"","description":"","icon":"","invisible":false}

wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
wonderflow
wonderflow reviewed Jul 22, 2022
View reviewed changes
pkg/addon/render.go Outdated Show resolved Hide resolved
@wangyikewxgm
pass tests
a1564b5 
Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

refactor some codes

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

WIP delete useless workflow

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

add checklegacy addon

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

refactor some logics

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix panic test

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

delete useless addon test

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix empty clusterargs

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix comments

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
@wangyikewxgm
rewrite some logic to support parameter ui
f28c342 
Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
@wangyikewxgm
rename template cue
39db065 
Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix panic test

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

add tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix render tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix checkdiff

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
@wangyikewxgm
add tests
29392be 
Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix

add more tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

add tests

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
@wangyikewxgm
update file header
2ed4cb2 
Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
@wangyikewxgm
fix comments
95bedda 
Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix comments

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>
@wonderflow wonderflow merged commit 25b0cb8 into kubevela:master Jul 22, 2022
This was referenced Jul 24, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlie0129 charlie0129 charlie0129 left review comments

@wonderflow wonderflow wonderflow approved these changes

@Somefive Somefive Somefive approved these changes

@barnettZQG barnettZQG Awaiting requested review from barnettZQG

@leejanee leejanee Awaiting requested review from leejanee

@zzxwill zzxwill Awaiting requested review from zzxwill

@StevenLeiZhang StevenLeiZhang Awaiting requested review from StevenLeiZhang StevenLeiZhang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Addon Install resources in needNamespace
4 participants
@wangyikewxgm @wonderflow @Somefive @charlie0129