Skip to content

Change externalElasticsearchUrl to externalElasticsearchHost at cc#185

Closed
mangoGoForward wants to merge 3 commits into
kubesphere:masterfrom
mangoGoForward:feat/externalElasticsearchHost
Closed

Change externalElasticsearchUrl to externalElasticsearchHost at cc#185
mangoGoForward wants to merge 3 commits into
kubesphere:masterfrom
mangoGoForward:feat/externalElasticsearchHost

Conversation

@mangoGoForward

Copy link
Copy Markdown
Member

What this PR dose?

Change externalElasticsearchUrl to externalElasticsearchHost at cc, and make the definition more clear.

What this PR does / why we need it:

please see kubesphere/kubesphere#3829.

Which issue(s) this PR fixes:

Fixes kubesphere/kubesphere#3829.

@ks-ci-bot ks-ci-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 4, 2021
@zheng1 zheng1 requested review from misteruly and pixiake November 4, 2021 12:38
@FeynmanZhou

Copy link
Copy Markdown

/cc @kubesphere/sig-observability

@mangoGoForward

Copy link
Copy Markdown
Member Author

/assign @calvinyv

@calvinyv

calvinyv commented Nov 5, 2021

Copy link
Copy Markdown

what about the users from the old version of KubeSphere CC with previous parm? do we have any migration mechanism?

@mangoGoForward

Copy link
Copy Markdown
Member Author

what about the users from the old version of KubeSphere CC with previous parm? do we have any migration mechanism?

We may could use --set to set spec.common.es.externalElasticsearchHost, or add some logic like this in

{{ lookup "installer.kubesphere.io/v1alpha1" "ClusterConfiguration" "kubesphere-system" "ks-installer" | toYaml }}

{{- $clusterConfiguration := lookup "installer.kubesphere.io/v1alpha1" "ClusterConfiguration" "kubesphere-system" "ks-installer" -}}
{{- if and $clusterConfiguration.spec.common.es.externalElasticsearchUrl (not $clusterConfiguration.spec.common.es.externalElasticsearchHost) }}
  {{ $clusterConfiguration.spec.common.es.externalElasticsearchHost = $clusterConfiguration.spec.common.es.externalElasticsearchUrl }}
{{ end -}}
{{ toYaml $clusterConfiguration }}

Could you give some suggestions?

@pixiake

pixiake commented Nov 8, 2021

Copy link
Copy Markdown
Contributor

Thanks for this contribution. And https://github.com/kubesphere/ks-installer/pull/1798 has been merged.

But v3.2.0 has been released and does not contain this change. Therefore, I think it is more appropriate to merge this pr and kubesphere/kubekey#742 after the release of the next version, otherwise it will cause v3.2.0 to fail to find externalElasticsearchUrl in the installation process.

@mangoGoForward

Copy link
Copy Markdown
Member Author

Thanks for this contribution. And kubesphere/ks-installer#1798 has been merged.

But v3.2.0 has been released and does not contain this change. Therefore, I think it is more appropriate to merge this pr and kubesphere/kubekey#742 after the release of the next version, otherwise it will cause v3.2.0 to fail to find externalElasticsearchUrl in the installation process.

Ok. And do we need some compatibility measures for old version like above logic?

@pixiake

pixiake commented Nov 8, 2021

Copy link
Copy Markdown
Contributor

Adding only the logic above should not be fully compatible with the old version, because the install process is already included in the image of the old version.

@ks-ci-bot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mangoGoForward
To complete the pull request process, please ask for approval from calvinyv after the PR has been reviewed.

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

Details 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

@mangoGoForward

Copy link
Copy Markdown
Member Author

Adding only the logic above should not be fully compatible with the old version, because the install process is already included in the image of the old version.

I have no idea to avoid now, if you have any idea, please share with me, Thanks a lot.

@pixiake

pixiake commented Nov 8, 2021

Copy link
Copy Markdown
Contributor

I think the best way at present may be to update the chart of ks-installer to v3.2.0 first, and then the later new version can use externalElasticsearchHost.

@FeynmanZhou

FeynmanZhou commented Nov 8, 2021

Copy link
Copy Markdown

I think the best way at present may be to update the chart of ks-installer to v3.2.0 first, and then the later new version can use externalElasticsearchHost.

@pixiake Do we need to create a GitHub issue to track the ks-installer Helm Chart upgrade? If so, it could be a good first issue.

@mangoGoForward

Copy link
Copy Markdown
Member Author

I think the best way at present may be to update the chart of ks-installer to v3.2.0 first, and then the later new version can use externalElasticsearchHost.

Ok.

@pixiake

pixiake commented Nov 11, 2021

Copy link
Copy Markdown
Contributor

Do we need to create a GitHub issue to track the ks-installer Helm Chart upgrade? If so, it could be a good first issue.

Yes,I have created an issue #190 .

@mangoGoForward

Copy link
Copy Markdown
Member Author

Do we still need to modify externalElasticsearchUrl to externalElasticsearchHost? @pixiake

@mangoGoForward mangoGoForward deleted the feat/externalElasticsearchHost branch July 22, 2022 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change externalElasticsearchUrl to externalElasticsearchHost at cc

5 participants