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

RHIDP-1703: Revert querying of OCP Cluster Ingress #9

Conversation

coreydaley
Copy link
Member

@coreydaley coreydaley commented Mar 21, 2024

Revert the automatic querying of the OCP Cluster Ingress for the domain to use for the clusterRouterBase as regular users do not have access to this resource, only cluster admins.

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The pre-commit utility can be used to generate the necessary content. Use pre-commit run -a to apply changes.
  • JSON Schema template updated and re-generated the raw schema via pre-commit hook.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Revert the automatic querying of the OCP Cluster Ingress for the domain
to use for the clusterRouterBase as regular users do not have access
to this resource, only cluster admins.
@coreydaley coreydaley requested review from a team as code owners March 21, 2024 15:15
@openshift-ci openshift-ci bot requested review from kadel and tumido March 21, 2024 15:15
Copy link

sonarcloud bot commented Mar 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Revert the automatic querying of the OCP Cluster Ingress for the domain to use for the clusterRouterBase as regular users do not have access to this resource, only cluster admins.

I guess the issue that the reverted commit was supposed to fix should be somehow reopened here or in JIRA as not fixed?
janus-idp#167?

@nickboldt
Copy link
Member

nickboldt commented Mar 21, 2024

Revert the automatic querying of the OCP Cluster Ingress for the domain to use for the clusterRouterBase as regular users do not have access to this resource, only cluster admins.

I guess the issue that the reverted commit was supposed to fix should be somehow reopened here or in JIRA as not fixed? janus-idp#167?

makes sense, yeah -- however janus-idp#167 is in an archived repo, so we need a new issue in https://github.com/redhat-developer/rhdh-chart/ ... but there are no issues for that repo because we use JIRA instead.

@coreydaley
Copy link
Member Author

coreydaley commented Mar 21, 2024

@rm3l From what I can see there is not a programmatical way to solve this issue due to limitations in Helm. The issue is really solved via the documentation that describes what information should be placed in the clusterRouterBase field. I was also thinking that maybe we could just have this code run if the user is a cluster-admin, but I don't see a way using helm to determine that as the install just fails on the lookup command, even if it is just used in an if block.

@nickboldt
Copy link
Member

nickboldt commented Mar 21, 2024

approved to make the GH spice flow, but I'm not an SME. recommend further scrutiny.
image

@coreydaley
Copy link
Member Author

Two approvals, I am going to go ahead and merge this.

@coreydaley coreydaley merged commit e3e1be3 into redhat-developer:main Mar 22, 2024
4 checks passed
@rm3l
Copy link
Member

rm3l commented Mar 22, 2024

@rm3l From what I can see there is not a programmatical way to solve this issue due to limitations in Helm. The issue is really solved via the documentation that describes what information should be placed in the clusterRouterBase field. I was also thinking that maybe we could just have this code run if the user is a cluster-admin, but I don't see a way using helm to determine that as the install just fails on the lookup command, even if it is just used in an if block.

No problem, just wanted to make sure that people who reported the original issue (janus-idp#167) were aware of this revert in case they expected the original issue to have been implemented (and were maybe depending on it), in which case this PR would have been a potential breaking change.
But after a closer look at janus-idp#167, it was requested in parodos-dev/orchestrator-helm-chart#12. But it seems from their Chart.yaml that they are no longer using the RHDH Chart, but are now using the RHDH Operator instead.
So I guess we don't need to create a JIRA issue for janus-idp#167

Roming22 added a commit to Roming22/rhtap-installer that referenced this pull request Mar 28, 2024
Roming22 added a commit to redhat-appstudio/rhtap-installer that referenced this pull request Mar 28, 2024
Roming22 added a commit to Roming22/rhtap-installer that referenced this pull request Apr 9, 2024
Roming22 added a commit to redhat-appstudio/rhtap-installer that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants