8000 feat: support merge cmd env with image extension by kakaZhou719 · Pull Request #2207 · sealerio/sealer · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: support merge cmd env with image extension #2207

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
Merged
merged 2 commits into from
May 16, 2023
Merged

feat: support merge cmd env with image extension #2207

merged 2 commits into from
May 16, 2023

Conversation

kakaZhou719
Copy link
Member
@kakaZhou719 kakaZhou719 commented Apr 28, 2023 •
edited
Loading

Describe what this PR does / why we need it

we have already supported set ENV and APPENV on Kubefile, each key-pairs will be saved in image extension.see : #2179.

so , how to use the default env with the user-defined env, That's what this PR does.

example:

Kubefile:

FROM scratch
ENV globalKey=globalValue
APP app1 local://app1
APP app2 local://app2
APP app3 local://app3
APPENV app1 key1=value1 key2=value2
LAUNCH ["app1","app2"]
  1. merge ENV value to v2.ClusterSpec.Env, if not present, just append it.
  2. merge APPENV value to v2.ApplicationConfig.Env, if not present, just append it.

priority order:

for cluster env:
user defined env from cmd flag -e > v2.ClusterSpec.Env > ImageExtension.Env

for app env:

v2.ApplicationConfig.Env > v2.ClusterSpec.Env > ImageExtension.Applications.Env

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@githu 8000 b-actions github-actions bot added the test label Apr 28, 2023
@kakaZhou719 kakaZhou719 force-pushed the merge-cmd-env-with-image-extension branch from 9c77916 to 25a1dc2 Compare April 29, 2023 10:03
@codecov-commenter
Copy link
codecov-commenter commented Apr 29, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: -6.80 ⚠️

Comparison is base (150c4cb) 19.93% compared to head (9e65814) 13.14%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2207      +/-   ##
==========================================
- Coverage   19.93%   13.14%   -6.80%     
==========================================
  Files          98      263     +165     
  Lines        9220    22696   +13476     
==========================================
+ Hits         1838     2983    +1145     
- Misses       7128    19308   +12180     
- Partials      254      405     +151
Flag Coverage Δ
e2e-tests 8.38% <ø> (?)
unit-tests 20.08% <100.00%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
cmd/sealer/cmd/utils/application.go 70.37% <100.00%> (+10.37%) ⬆️
cmd/sealer/cmd/utils/cluster.go 36.31% <100.00%> (+2.59%) ⬆️
pkg/application/v2app.go 42.76% <100.00%> (+1.15%) ⬆️

... and 165 files with indirect coverage changes

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

@kakaZhou719 kakaZhou719 force-pushed the merge-cmd-env-with-image-extension branch from 3439b80 to cdb8e1f Compare May 15, 2023 03:31
starnop
starnop reviewed May 15, 2023
View reviewed changes
cmd/sealer/cmd/utils/cluster.go
)

// MergeClusterWithImageExtension :set default value get from image extension,such as image global env
func MergeClusterWithImageExtension(cluster *v2.Cluster, imageExt imagev1.ImageExtension) *v2.Cluster {
if len(imageExt.Env) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

return fast

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought there will be another args to be merged later.

starnop
starnop reviewed May 15, 2023
View reviewed changes
pkg/application/v2app.go Outdated
@@ -22,6 +22,8 @@ import (
"strings"
"syscall"

mapUtils "github.com/sealerio/sealer/utils/maps"

Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this blank

starnop
starnop reviewed May 15, 2023
View reviewed changes
cmd/sealer/cmd/utils/application.go Outdated
@@ -20,7 +20,7 @@ import (
)

//ConstructApplication merge flags to v2.Application
func ConstructApplication(app *v2.Application, cmds, appNames []string) *v2.Application {
func ConstructApplication(app *v2.Application, cmds, appNames, appEnvs []string) *v2.Application {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/appEnvs/globalEnvs?

WDYT?

starnop
starnop reviewed May 15, 2023
View reviewed changes
cmd/sealer/cmd/cluster/apply.go
// set merged cluster
cf.SetCluster(*cluster)
cf.SetCluster(*mergedWithExt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if the Clusterfile saved to the kubernetes cluster will change?

Copy link
Member Author

Choose a reason for hiding this comment

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

mergedWithExt is an expected value, it will not change the cluster which already saved.

kakzhou719 and others added 2 commits May 15, 2023 18:13
@kakaZhou719
feat: support merge cmd env with image extension
0cd1a5b 
Signed-off-by: kakzhou719 <kakazhou719@163.com>

feat: support merge cmd env with image extension

Signed-off-by: kakzhou719 <kakazhou719@163.com>

feat:support env and appenv instruction
@kakaZhou719
feat:support env and appenv instruction
b6b4523
@kakaZhou719 kakaZhou719 force-pushed the merge-cmd-env-with-image-extension branch from 9e65814 to b6b4523 Compare May 15, 2023 10:14
starnop
starnop approved these changes May 16, 2023
View reviewed changes
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

@starnop starnop merged commit d83ead0 into sealerio:main May 16, 2023
@open-digger-bot open-digger-bot bot mentioned this pull request May 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@starnop starnop starnop approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@kakaZhou719 @codecov-commenter @starnop
0