Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

chore(#624): change config parameters to use for redirect url #625

Closed

Conversation

dipak-pawar
Copy link
Contributor

Fixes: #624

@dipak-pawar dipak-pawar requested a review from sbose78 August 28, 2018 10:06
@alien-ike
Copy link

Ike Plugins (test-keeper)

Thank you @dipak-pawar for this contribution!

It appears that no tests have been added or updated in this PR.

Automated tests give us confidence in shipping reliable software. Please add some as part of this change.

If you are an admin or the reviewer of this PR and you are sure that no test is needed then you can use the command /ok-without-tests as a comment to make the status green.

For more information please head over to official documentation. You can find there how to configure the plugin.

@fabric8cd
Copy link

@dipak-pawar snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-625-1

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #625 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #625   +/-   ##
=======================================
  Coverage   72.79%   72.79%           
=======================================
  Files          84       84           
  Lines        8485     8485           
=======================================
  Hits         6177     6177           
  Misses       1835     1835           
  Partials      473      473

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec4d00b...a12868f. Read the comment docs.

Copy link
Contributor

@xcoulon xcoulon left a comment

Choose a reason for hiding this comment

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

Looks like you have 2 changes that have nothing in common in this PR ?

@@ -75,6 +75,11 @@ objects:
configMapKeyRef:
name: ${SERVICE_NAME}
key: notification.serviceurl
- name: AUTH_UI_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

if we really need such a URL configured in auth (which I'm not really a fan of), then we'll also have to declare it in https://github.com/fabric8-services/fabric8-auth/blob/master/openshift/auth.app.yaml#L31

@@ -33,3 +33,5 @@ objects:
data:
wit.url: http://wit
notification.serviceurl: http://f8notification
# you can set "ui.url" to your locally deployed ui. by default it's localhost:8080
# ui.url: http://ui
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume there's a variable with the default value set to localhost:8080 declared in https://github.com/fabric8-services/fabric8-auth/blob/master/configuration/configuration.go ?

@@ -50,7 +50,7 @@ objects:
notapproved_redirect: ""
keycloak.url: https://sso.openshift.io
notification.serviceurl: ""
email.verify.url: https://prod-preview.openshift.io/_home
invitation.accepted.url: https://prod-preview.openshift.io/_home
email.verify.url: https://openshift.io/_redirects/_verifyEmail
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, this file is just an example. If there's something to fix on prod and pre-prod, we'll have to open a HK to update the configmaps

@sbose78
Copy link
Member

sbose78 commented Sep 3, 2018

These should only be the defaults.

@sbose78
Copy link
Member

sbose78 commented Sep 4, 2018

@dipak-pawar Should we close this?

@dipak-pawar dipak-pawar closed this Sep 4, 2018
@fabric8cd
Copy link

@dipak-pawar snapshot fabric8-auth image is available for testing. docker pull docker.io/fabric8/fabric8-auth:SNAPSHOT-PR-625-2

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

Successfully merging this pull request may close these issues.

5 participants