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

artifactory - Fix for ClusterIP type service with external ingress offloading #1906

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fwdIT
Copy link

@fwdIT fwdIT commented Jul 25, 2024

Fixes current helm chart limitations for external ingress with nginx ClusterIP service type and https / tls offloading on external ingress

In our case basic OpenShift 4.12.x install where the ingress is not the standard nginx which comes with the helm install

PR Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Chart Version bumped
  • CHANGELOG.md updated
  • Variables and other changes are documented in the README.md
  • Title of the PR starts with chart name (e.g. [artifactory])

What this PR does / why we need it:
Support for OpenShift type installations

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes # 308262 (support case @ JFrog)

Special notes for your reviewer:
discussed with [email protected]

Copy link

github-actions bot commented Jul 25, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@fwdIT
Copy link
Author

fwdIT commented Jul 25, 2024

I have read the CLA Document and I hereby sign the CLA

@oumkale
Copy link
Member

oumkale commented Jul 30, 2024

Hi @fwdIT,

Thank you for raising PR, we will take this internally and fix this in upcoming releases.

We will update you

@oumkale oumkale self-requested a review July 30, 2024 09:24
@fwdIT
Copy link
Author

fwdIT commented Sep 25, 2024

Any update on this? For us it won't be optimal to start with manually tweaked upstream helm charts for our automated installation. During the POC, this was no issue. We however now, since we are licensed, need to automate the install and we don't want to use custom adjusted vendor helm charts for this. We should only need to adjust the values, not the logic.

@RobinDuhan
Copy link

@fwdIT We have taken this, and it will be released tentatively by third week of October. I have raised another PR on top of your changes - #1926 - please test and verify if this works for your use case.

@RobinDuhan
Copy link

@fwdIT Reminder to review

@fwdIT
Copy link
Author

fwdIT commented Oct 3, 2024

@RobinDuhan sorry for the delay, looks fine by me
took a dry run from my previous code and again from your commit and aside from some new blocks, like jfrog-platform-artifactory-configmaps (and some parts in comments), minor image version changes, the main parts and certainly the nginx part stays identical and what is needed to work on OpenShift

thx for the tweaks and additions! this will help us not needing to use manually adjusted helm charts like today

looking forward to the release

@RobinDuhan
Copy link

@fwdIT Great, thank you for verifying this at your end as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants