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

Fix Helm Broker port names in services to comply with Istio convention #13065

Merged

Conversation

mjakobczyk
Copy link
Contributor

Description

We should follow official Istio documentation about port naming convention. After editing both services there is a similar spec which indicates that the used protocol is TCP.

...
spec:
  ...
  ports:
  - name: client
    port: 2379
    protocol: TCP
    targetPort: 2379
  - name: metrics
    port: 2381
    protocol: TCP
    targetPort: 2381
  ...

Changes proposed in this pull request:

  • fix port names in helm-broker-etcd-stateful service to include protocol prefix
  • fix port names in helm-broker-etcd-stateful-client service to include protocol prefix
  • get rid of the following errors:
Info [IST0118] (Service helm-broker-etcd-stateful-client.kyma-system) Port name client (port: 2379, targetPort: 2379) doesn't follow the naming convention of Istio port.
Info [IST0118] (Service helm-broker-etcd-stateful-client.kyma-system) Port name metrics (port: 2381, targetPort: 2381) doesn't follow the naming convention of Istio port.
Info [IST0118] (Service helm-broker-etcd-stateful.kyma-system) Port name client (port: 2379, targetPort: 2379) doesn't follow the naming convention of Istio port.
Info [IST0118] (Service helm-broker-etcd-stateful.kyma-system) Port name peer (port: 2380, targetPort: 2380) doesn't follow the naming convention of Istio port.

Related issue(s)

See: #11686

@kyma-bot kyma-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 13, 2022
@mjakobczyk mjakobczyk added the area/service-management Issues or PRs related to service management label Jan 13, 2022
@mjakobczyk
Copy link
Contributor Author

/retest

@kyma-bot kyma-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 26, 2022
@netlify
Copy link

netlify bot commented Jan 26, 2022

✔️ 🥰 Documentation preview ready! 🥰

🔨 Explore the source changes: b847beb

🔍 Inspect the deploy log: https://app.netlify.com/sites/kyma-project-docs-preview/deploys/61f12f80d48b0a0007729b84

😎 Browse the preview: https://deploy-preview-13065--kyma-project-docs-preview.netlify.app

Copy link
Contributor

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

given helm broker is deprecated and will be completely removed from kyma, I would like to propose we don't touch it unless absolutely necessary.

@strekm
Copy link
Contributor

strekm commented Jan 26, 2022

@wozniakjan our motivation is to fix all istioctl analyze warnings/infos and to use that in command in pipeline. i wouldn't say this change is harmful or dangerous in any way, more i would say it's cosmetics

@mjakobczyk
Copy link
Contributor Author

/test pre-main-kyma-gardener-azure-alpha-prod

@kyma-bot
Copy link
Contributor

@mjakobczyk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pre-main-kyma-gardener-azure-alpha-prod b847beb link false /test pre-main-kyma-gardener-azure-alpha-prod

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@kyma-bot kyma-bot added the lgtm Looks good to me! label Jan 26, 2022
@kyma-bot kyma-bot merged commit 0b3aea2 into kyma-project:main Jan 26, 2022
@mjakobczyk mjakobczyk deleted the fix/istioctl-analyze/helm-broker branch January 27, 2022 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/service-management Issues or PRs related to service management lgtm Looks good to me! size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants