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

support launch app via its name though clusterfile or cli #1917

Merged
merged 2 commits into from
Dec 16, 2022

Conversation

kakaZhou719
Copy link
Member

Describe what this PR does / why we need it

sealer store some basic information about the cluster image in the image's annotations, such as cluster image type, applications list and its launch commands in one field .

however, we cannot accurately find the application launch command based on its name and all launch commands generate from "APP" instruction is mixed with the "CMDS" instruction in "launch.cmds" filed .

Therefore, I recommend that we only save the relevant information of the application during the build stage and control the launch logic of the application image during the run stage.

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

apiVersion: sealer.cloud/v2
kind: Cluster
metadata:
  creationTimestamp: null
  name: my-cluster
spec:
  appNames: [ "nginx","mysql"]
  hosts:
  - ips:
    - 172.16.26.162
    roles:
    - master
    ssh: {}
  image: registry.cn-qingdao.aliyuncs.com/sealer-io/kubernetes:v1.22.15
  registry: {}
  ssh:
    passwd: Seadent123
    pk: /root/.ssh/id_rsa
    port: "22"
    user: root
status: {}

Special notes for reviews

@github-actions github-actions bot added Clusterfile ImageBuilding related to all staff with image building test labels Dec 16, 2022
Hosts []Host `json:"hosts,omitempty"`
SSH v1.SSH `json:"ssh,omitempty"`
// APPNames This field allows user to specify the app name they want to run launch.
APPNames []string `json:"appNames"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

add omitempty tag?

func GetImageDefaultLaunchCmds(extension v12.ImageExtension) []string {
rawCmds := extension.Launch.Cmds
appNames := extension.Launch.AppNames
// if raw cmds exist in extension, return it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion, the launch applications should take precedence over cmds, and we can even mark cmd deprecated in the future because the app can completely override cmd's capabilities

@kakaZhou719 kakaZhou719 force-pushed the feat-support-run-app-via-appname branch from 0911fec to 19e13a3 Compare December 16, 2022 04:57
},
}

appFlags = &types.APPFlags{}
runAppCmd.Flags().StringSliceVar(&appFlags.LaunchCmds, "cmds", []string{}, "override default LaunchCmds of clusterimage")
runAppCmd.Flags().StringSliceVar(&appFlags.Cmds, "cmds", []string{}, "override default LaunchCmds of clusterimage")
runAppCmd.Flags().StringSliceVar(&appFlags.AppNames, "app-names", []string{}, "override default AppNames of clusterimage")
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/app-names/apps

@kakaZhou719 kakaZhou719 force-pushed the feat-support-run-app-via-appname branch from 19e13a3 to f0abbe3 Compare December 16, 2022 06:20
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Base: 21.52% // Head: 21.37% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (328fde4) compared to base (92521b9).
Patch coverage: 16.98% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1917      +/-   ##
==========================================
- Coverage   21.52%   21.37%   -0.16%     
==========================================
  Files          80       80              
  Lines        7415     7430      +15     
==========================================
- Hits         1596     1588       -8     
- Misses       5605     5629      +24     
+ Partials      214      213       -1     
Impacted Files Coverage Δ
build/kubefile/parser/app_handler.go 45.94% <ø> (-1.43%) ⬇️
pkg/cluster-runtime/apps.go 0.00% <0.00%> (ø)
pkg/cluster-runtime/installer.go 0.00% <0.00%> (ø)
pkg/infradriver/ssh_infradriver.go 39.93% <33.33%> (-0.07%) ⬇️
build/kubefile/parser/kubefile.go 55.84% <75.00%> (+2.39%) ⬆️
build/kubefile/parser/utils.go 60.75% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@starnop starnop left a comment

Choose a reason for hiding this comment

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

LGTM

@kakaZhou719 kakaZhou719 force-pushed the feat-support-run-app-via-appname branch from f0abbe3 to 328fde4 Compare December 16, 2022 10:20
@@ -230,7 +230,7 @@ func getApplicationType(sources []string) (string, []string, error) {

ish, helmErr := isHelm(sources...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/ish/isHelm

@starnop starnop merged commit ee33b05 into sealerio:main Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clusterfile ImageBuilding related to all staff with image building test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants