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: Support specifying publishVersion when creating or updating oam applications #888

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

iambocai
Copy link
Contributor

@iambocai iambocai commented Nov 21, 2023

Description of your changes

  1. Support specifying publishVersion when creating or updating oam applications via API
  2. update version in metadata.yaml from v1.8.0-rc.3 to v1.8.0-rc.4

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 yarn lint to ensure the frontend changes are ready for review.
  • Run make reviewableto ensure the server changes are ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

Special notes for your reviewer

  1. Because the requirement to specify publishVersion usually only exists during API calls, supporting users to specify this parameter on the front-end can actually lead to unnecessary understanding costs, so we didn't add support for specifying relevant parameters on the UX front-end.
  2. The VelaUX version indicated in the metadata.yaml file is 1.8.0-rc3, but in reality, the available version has already reached 1.9.3. Should I also change this to 1.9.3 or 1.9.4 (instead of v1.8.0-rc.4)?

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (2ab672c) 59.01% compared to head (da0e72c) 59.05%.

Files Patch % Lines
pkg/server/domain/service/oam_application.go 77.77% 3 Missing and 1 partial ⚠️
pkg/server/interfaces/api/oam_application.go 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #888      +/-   ##
==========================================
+ Coverage   59.01%   59.05%   +0.03%     
==========================================
  Files         116      116              
  Lines       19846    19863      +17     
==========================================
+ Hits        11713    11730      +17     
- Misses       6696     6697       +1     
+ Partials     1437     1436       -1     
Flag Coverage Δ
apiserver-unittests 32.63% <53.84%> (+0.01%) ⬆️
server-e2e-tests 48.37% <80.76%> (+0.05%) ⬆️

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.

@iambocai
Copy link
Contributor Author

Additionally, as I urgently need this feature in my production environment, I would like to release a new version of VelaUX

@@ -103,6 +103,9 @@ make e2e-server-test
make build-swagger
```

if you prefer v3 version of OpenAPI schema, please convert it
Copy link
Member

Choose a reason for hiding this comment

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

Please separate openAPI v3 update to another PR. Let's focus on publishVersion in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, no problem, just wait a moment~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please separate openAPI v3 update to another PR. Let's focus on publishVersion in this one.

Done

Copy link
Member

@FogDong FogDong left a comment

Choose a reason for hiding this comment

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

Since this PR only contains API changes, does it mean that we can not specify publishVersion in the frontend?

@iambocai
Copy link
Contributor Author

iambocai commented Nov 22, 2023

Since this PR only contains API changes, does it mean that we can not specify publishVersion in the frontend?

Yes, I am considering this: if users are supported to fill in PublishVersion on a web page, then for web users, they need to understand what PublishVersion is, why uniqueness needs to be guaranteed, and its relationship with Revision (currently, without specifying PublishVersion, the default value of PublishVersion is the same as Revision), This will bring two problems:

  • firstly, it brings some learning costs to web (usually beginner) users;
  • Secondly, they are easy to fill in duplicates, the current way(auto decided in server side) is more simple and reliable for them.

Of course, if necessary, I can also add front-end support.

上郡 added 4 commits November 22, 2023 14:23
…g or updating oam applications

Signed-off-by: 上郡 <[email protected]>
* 'feature/app_publish_version' of github.com:iambocai/velaux:
  fix: dryrun annotations fix
  fix: add annotations on update
  fix kubevela#713: duplicate field 'runtime_cluster' in swagger.json
  feat(apidoc): Add swagger-3.0.json
  feat(oam_application): Support specifying publishVersion when creating or updating oam applications
…g or updating oam applications

Signed-off-by: 上郡 <[email protected]>
* 'feature/app_publish_version' of github.com:iambocai/velaux:
  feat(oam_application): Support specifying publishVersion when creating or updating oam applications
# 请输入一个提交信息以解释此合并的必要性,尤其是将一个更新后的上游分支
# 合并到主题分支。
#
# 以 '#' 开始的行将被忽略,而空的提交说明将终止提交。
Copy link
Member

@FogDong FogDong left a comment

Choose a reason for hiding this comment

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

Generally LGTM if this feature is only supported in API.
Any insights? cc @barnettZQG

@barnettZQG
Copy link
Collaborator

@iambocai I think this change don't impact on the UI. LGTM after passed the CI checks.

@iambocai
Copy link
Contributor Author

iambocai commented Nov 27, 2023

@iambocai I think this change don't impact on the UI. LGTM after passed the CI checks.

thanks for your reply, I saw some test faild in ci, but all of them seems to be environment problems, may be we should fix them in another commit?

VelaUX APIServer Test / server-e2e-tests (v1.21) (pull_request) —— No matching K3s versions were found. (v1.21 is no longer supported by k3s)
image
the avaliable k3s versions started at v22, but we request v21.

@iambocai
Copy link
Contributor Author

iambocai commented Nov 27, 2023

@iambocai I think this change don't impact on the UI. LGTM after passed the CI checks.

I have upgrade the minor server-e2e-tests k8s version to 1.22 and passed the check, but ci still expect v1.21 test's result, may be we should modify git settings too?
image

@@ -91,7 +91,7 @@ jobs:
with:
version: 3.1.0
kubebuilderOnly: false
kubernetesVersion: v1.21.2
kubernetesVersion: v1.22.17
Copy link
Member

Choose a reason for hiding this comment

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

I think 1.21 is the oldest version we need to support, right? cc @chivalryq

Copy link
Contributor

Choose a reason for hiding this comment

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

we'd better revert the change back to 1.21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we should find some way to make the k3s setup works again, currently the k3s setup script only support latest 200 versions ( which >= v1.24.7-rc3+k3s1, and continue growth day by day):

image

curl --silent --fail --location "${authz[@]-}"  "https://api.github.com/repos/k3s-io/k3s/releases?per_page=999&page=2"

image

maybe we should find another k3s setup script to workaround this issue.

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.

4 participants