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

Feat: Add short name velaapp for application CRD #3816

Merged
merged 1 commit into from
May 7, 2022

Conversation

JarHMJ
Copy link
Member

@JarHMJ JarHMJ commented May 5, 2022

Description of your changes

Add short name velaapp for application CRD (#3797)

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

@wonderflow

Copy link
Collaborator

@barnettZQG barnettZQG left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #3816 (936da84) into master (ba0c226) will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3816      +/-   ##
==========================================
+ Coverage   63.91%   64.04%   +0.13%     
==========================================
  Files         312      312              
  Lines       29530    29561      +31     
==========================================
+ Hits        18874    18933      +59     
+ Misses       8204     8182      -22     
+ Partials     2452     2446       -6     
Flag Coverage Δ
apiserver-unittests 35.30% <ø> (+0.07%) ⬆️
core-unittests 56.53% <ø> (+0.39%) ⬆️
e2e-multicluster-test 26.70% <ø> (-0.07%) ⬇️
e2e-rollout-tests 23.62% <ø> (-0.06%) ⬇️
e2etests 31.30% <ø> (+0.13%) ⬆️

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

Impacted Files Coverage Δ
apis/core.oam.dev/v1alpha2/application_types.go 100.00% <ø> (ø)
apis/core.oam.dev/v1beta1/application_types.go 4.54% <ø> (ø)
pkg/apiserver/rest/webservice/oam_application.go 64.17% <0.00%> (-5.98%) ⬇️
pkg/apiserver/rest/utils/bcode/bcode.go 50.00% <0.00%> (-2.95%) ⬇️
pkg/workflow/tasks/custom/task.go 74.09% <0.00%> (-1.04%) ⬇️
pkg/apiserver/model/user.go 83.05% <0.00%> (ø)
...es/policydefinition/policydefinition_controller.go 69.47% <0.00%> (ø)
...ller/core.oam.dev/v1alpha2/application/revision.go 73.40% <0.00%> (+0.09%) ⬆️
pkg/apiserver/rest/usecase/application.go 63.06% <0.00%> (+0.58%) ⬆️
pkg/controller/utils/capability.go 79.42% <0.00%> (+0.88%) ⬆️
... and 6 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 ba0c226...936da84. Read the comment docs.

@wonderflow
Copy link
Collaborator

Please run make reviewable to fix the ci

@fourierr
Copy link
Member

fourierr commented May 6, 2022

lgtm

@@ -82,7 +82,7 @@ type ApplicationSpec struct {
// Application is the Schema for the applications API
// +kubebuilder:storageversion
// +kubebuilder:subresource:status
// +kubebuilder:resource:categories={oam},shortName=app
// +kubebuilder:resource:categories={oam},shortName={app,velaapp}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the short name didn't change the CRD file? Does it really work?

Copy link
Member Author

Choose a reason for hiding this comment

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

go mod tidy
git --no-pager diff
diff --git a/charts/vela-core/crds/core.oam.dev_applications.yaml b/charts/vela-core/crds/core.oam.dev_applications.yaml
index 9d4fc00..82aa353 100644
--- a/charts/vela-core/crds/core.oam.dev_applications.yaml
+++ b/charts/vela-core/crds/core.oam.dev_applications.yaml
@@ -27,7 +27,6 @@ spec:
     plural: applications
     shortNames:
     - app
-    - velaapp
     singular: application
   scope: Namespaced
   versions:
diff --git a/charts/vela-minimal/crds/core.oam.dev_applications.yaml b/charts/vela-minimal/crds/core.oam.dev_applications.yaml
index 9d4fc00..82aa353 100644
--- a/charts/vela-minimal/crds/core.oam.dev_applications.yaml
+++ b/charts/vela-minimal/crds/core.oam.dev_applications.yaml
@@ -27,7 +27,6 @@ spec:
     plural: applications
     shortNames:
     - app
-    - velaapp
     singular: application
   scope: Namespaced
   versions:
git diff --quiet || (echo `date +%H:%M:%S` [FAIL] please run 'make reviewable' to include all changes && false)
19:35:09 [FAIL] please run make reviewable to include all changes
make: *** [Makefile:57: check-diff] Error 1

This is the result of the last CI which confuse me. It seems that after I have changed the CRD, the CI report the err.

Copy link
Collaborator

Choose a reason for hiding this comment

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

do you run make reviewable

Copy link
Collaborator

Choose a reason for hiding this comment

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

the make reviewable will invoke go generate to generate CRD with the help of controller runtime. If it removes the like in the CRD, maybe that's not take affect to write like that.

Copy link
Collaborator

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

have you tested locally, does it really work like this?

@JarHMJ
Copy link
Member Author

JarHMJ commented May 6, 2022

have you tested locally, does it really work like this?

我发现了一个比较诡异的问题,我执行go generate ./pkg/... ./apis/... 这个命令时,有时候会修改crd文件中shortNames这个字段,有时候却又不会。然后我排查是否controller-gen版本不兼容,我升级到v0.8.0时还是有这个问题。麻烦能看一下这个生成的问题么? @wonderflow

@wonderflow
Copy link
Collaborator

@JarHMJ

有两处 Application spec 要改。

You need to change both v1alpha2 and v1beta1 for application spec.

Signed-off-by: huangminjie <minjie.huang@daocloud.io>
@wonderflow wonderflow merged commit d25676a into kubevela:master May 7, 2022
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.

None yet

4 participants