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

Deploy OpenShift route for external communications using the Helm Chart #3777

Closed
wants to merge 6 commits into from
Closed

Deploy OpenShift route for external communications using the Helm Chart #3777

wants to merge 6 commits into from

Conversation

rmarting
Copy link

@rmarting rmarting commented May 4, 2023 •
edited
Loading

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)

This PR is created to support the enhacement identified in the provectus/kafka-ui-charts#4. It is an initial approach to cover that feature for OpenShift environments.

Is there anything you'd like reviewers to focus on?

Code format for the new Route template (I compared with the Ingress one, but just to double check that is aligned with the code formats)

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

It was tested locally using a local OpenShift cluster (using OpenShift Local), and another remote cluster.

The value.yaml file used for my local tests was similar to:

yamlApplicationConfig:
  kafka:
    clusters:
      - name: my-cluster
        bootstrapServers: my-kafka-kafka-bootstrap.amq-streams-demo.svc:9092
  auth:
    type: disabled
  management:
    health:
      ldap:
        enabled: false

ingress:
  enabled: false
  host: kafka-ui-amq-streams-demo.apps-crc.testing

route:
  enabled: true

This is the command used to execute the Helm Chart:

helm upgrade --install kafka-ui charts/kafka-ui/ -f values.yaml --history-max 2

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

@rmarting rmarting requested a review from a team as a code owner May 4, 2023 13:22
github-actions[bot]
github-actions bot reviewed May 4, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there rmarting! 👋

Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖

We will try to review it soon!

This was referenced May 4, 2023
Open
Merged
@Haarolean
Copy link
Contributor

@Narekmat @provectus/kafka-devops PTAL :)

Narekmat
Narekmat requested changes May 10, 2023
View reviewed changes
Copy link
Collaborator

@Narekmat Narekmat left a comment

Choose a reason for hiding this comment

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

@rmarting thank you for your changes.
Please increment version in Chart.yaml file to 0.6.4

@Haarolean Haarolean assigned rmarting and unassigned Narekmat May 11, 2023
@Haarolean Haarolean added type/enhancement En enhancement to an already existing feature status/pending Further information is requested scope/k8s K8s or helm stuff (really annoying) labels May 11, 2023
@rmarting
Copy link
Author

rmarting commented May 12, 2023 •
edited
Loading

@Narekmat Done!!! Ready for your review! Thanks

However, it seems that there is a conflict with the latest version in the master branch where the version defined is the 0.7.0 ... so should I upgrade the Chart.yaml file to 0.7.x?

@rmarting rmarting requested a review from Narekmat May 15, 2023 16:58
@cadnce
Copy link

cadnce commented May 16, 2023

Are you aware that you can use ingress annotations to achieve the same effect? :)
I.e. route.openshift.io/termination: reencrypt

In my experience it is rare to include openshift specific features such as routes in generic helm charts.

@rmarting
Copy link
Author

@cadnce you are right, however OpenShift routes is another way to expose externally services outside from the platform. I found many users using OpenShift routes to expose their services instead of the ingress objects, as from a platform point of view, it is very easy and you don't need to deal with the host entries.

This PR is just to improve the helm chart to use this kind of object (optionally) for this kind of platform. I found other Helm Charts in the community including too.

@rmarting
Copy link
Author

@Narekmat Done!!! Ready for your review! Thanks

However, it seems that there is a conflict with the latest version in the master branch where the version defined is the 0.7.0 ... so should I upgrade the Chart.yaml file to 0.7.x?

Any comment? Thanks!

Narekmat
Narekmat requested changes May 19, 2023
View reviewed changes
Copy link
Collaborator

@Narekmat Narekmat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please replace appVersion: to v0.7.0 and version 0.7.2

@rmarting
Copy link
Author

Please replace appVersion: to v0.7.0 and version 0.7.2

@Narekmat Done!

@github-actions
Copy link

This PR has been automatically marked as stale because no requested changes have been applied. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the status/stale Stale issues will be closed in 7 days. label May 27, 2023
@rmarting rmarting requested a review from Narekmat May 27, 2023 11:16
@rmarting
Copy link
Author

@Narekmat This PR is ready for your review. Thanks!

@github-actions github-actions bot removed status/stale Stale issues will be closed in 7 days. status/pending Further information is requested labels May 28, 2023
@Haarolean
Copy link
Contributor

We'll take a look why helm linter is failing.

@Haarolean
Copy link
Contributor

@rmarting it seems that this logic is indeed very dependent on openshift, as our linter cannot validate the changes.
I suggest closing this PR unless you know a way to satisfy our linter :)

@Haarolean Haarolean added the status/pending Further information is requested label Jun 16, 2023
@github-actions
Copy link

This PR has been automatically marked as stale because no requested changes have been applied. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the status/stale Stale issues will be closed in 7 days. label Jun 28, 2023
@rmarting
Copy link
Author

@Haarolean please, dont close for now this PR. I have an idea about how to validate the route object, but I need some time to implement it. How long can be opened the PR before an automatic close? Thanks

@Haarolean
Copy link
Contributor

@rmarting we'd need to reopen it in the charts repo anyway :)

@github-actions github-actions bot removed status/stale Stale issues will be closed in 7 days. status/pending Further information is requested labels Jun 29, 2023
@Haarolean
Copy link
Contributor

https://github.com/provectus/kafka-ui-charts

@Haarolean Haarolean closed this Jul 24, 2023
@rmarting
Copy link
Author

Ok! @Haarolean I will try to resolve it there with a new PR. Thanks for the heads up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@Narekmat Narekmat Awaiting requested review from Narekmat

Assignees

@rmarting rmarting

Labels
scope/k8s K8s or helm stuff (really annoying) type/enhancement En enhancement to an already existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for OpenShift routes in Helm Chart
4 participants
@rmarting @Haarolean @cadnce @Narekmat