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

Support config for container image #285

Merged
merged 9 commits into from
Jul 16, 2022
Merged

Support config for container image #285

merged 9 commits into from
Jul 16, 2022

Conversation

liangyuanpeng
Copy link
Member

@liangyuanpeng liangyuanpeng commented Jun 24, 2022 •
edited by renyunkang
Loading

Description

Add param of keepalived-image for openelb manager
You can specify all images used by openelb through configmap. Name must be openelb-images.

apiVersion: v1
kind: ConfigMap
metadata:
  name: openelb-images
  namespace: openelb-system
data:
  forward-image: kubesphere/openelb-forward:master
  proxy-image: kubesphere/openelb-proxy:master
  keepalived-vip-image: kubesphere/kube-keepalived-vip:0.35

Related links:

#283 #214

@ks-ci-bot ks-ci-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 24, 2022
@liangyuanpeng liangyuanpeng force-pushed the Support_config_keepalivedImage branch from f9e1eb1 to 464af86 Compare June 24, 2022 10:43
@renyunkang
Copy link
Member

we don't have the image kubesphere/openelb-forward:v0.5.0 and kubesphere/openelb-proxy:v0.5.0.

@renyunkang
Copy link
Member

How about specifying the keepalived image through ConfigMap, like openelb-proxy and openelb-forward, it is convenient for us to manage it uniformly.

@renyunkang
Copy link
Member

kindly ping @liangyuanpeng

@liangyuanpeng
Copy link
Member Author

liangyuanpeng commented Jul 6, 2022 •
edited
Loading

How about specifying the keepalived image through ConfigMap, like openelb-proxy and openelb-forward, it is convenient for us to manage it uniformly.

Make sense, let me update it.

@ks-ci-bot ks-ci-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 11, 2022
@renyunkang
Copy link
Member

renyunkang commented Jul 12, 2022 •
edited
Loading

Thank you very much for your contribution. I was wondering if we could use a configmap to specify all the custom images used by openelb. For example: configmap openelb-image-configmap defines forward-image/proxy-image/keepalived-vip-image and other images used by openelb. In addition, whether we still need to provide command line parameters to specify the image after specifying the images through configmap?

chaunceyjiang
chaunceyjiang reviewed Jul 15, 2022
View reviewed changes
pkg/speaker/vip/keepalived.go Outdated
if err != nil {
return constant.OpenELBDefaultKeepAliveImage
}
return cm.Data[constant.OpenELBKeepAliveImage]
Copy link
Contributor

Choose a reason for hiding this comment

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

cm.Data[constant.OpenELBKeepAliveImage]
Is it possible for null pointer to occur here?

Copy link
Member

Choose a reason for hiding this comment

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

oh, may be empty

@chaunceyjiang
Copy link
Contributor

Thanks!

/lgtm

@ks-ci-bot ks-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2022
@ks-ci-bot
Copy link
Collaborator

LGTM label has been added.

Git tree hash: 7b5ae8bc050cd563e287bdb32c7638196971d80e

1 similar comment
@ks-ci-bot
Copy link
Collaborator

LGTM label has been added.

Git tree hash: 7b5ae8bc050cd563e287bdb32c7638196971d80e

@liangyuanpeng liangyuanpeng changed the title Add param of keepalived-image for openelb manager Support config for container image Jul 16, 2022
@liangyuanpeng
Copy link
Member Author

Thanks for your work.

/lgtm

whether we still need to provide command line parameters to specify the image after specifying the images through configmap?

Some Suggest ,Only configmap or some config file should be enought.

@ks-ci-bot
Copy link
Collaborator

@liangyuanpeng: you cannot LGTM your own PR.

In response to this:

Thanks for your work.

/lgtm

whether we still need to provide command line parameters to specify the image after specifying the images through configmap?

Some Suggest ,Only configmap or some config file should be enought.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

1 similar comment
@ks-ci-bot
Copy link
Collaborator

@liangyuanpeng: you cannot LGTM your own PR.

In response to this:

Thanks for your work.

/lgtm

whether we still need to provide command line parameters to specify the image after specifying the images through configmap?

Some Suggest ,Only configmap or some config file should be enought.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@renyunkang
Copy link
Member

/lgtm
/approve

@ks-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liangyuanpeng, renyunkang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@ks-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liangyuanpeng, renyunkang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ks-ci-bot ks-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2022
@ks-ci-bot ks-ci-bot merged commit b6b6f11 into openelb:master Jul 16, 2022
@renyunkang renyunkang mentioned this pull request Sep 26, 2022
Open
This was referenced Dec 14, 2023
Closed
Closed
@bdowling bdowling mentioned this pull request Mar 6, 2024
Open
@renyunkang renyunkang mentioned this pull request Jun 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renyunkang renyunkang renyunkang left review comments

@chaunceyjiang chaunceyjiang chaunceyjiang left review comments

Assignees

@chaunceyjiang chaunceyjiang

@renyunkang renyunkang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liangyuanpeng @renyunkang @chaunceyjiang @ks-ci-bot