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

Include the Log Out option when a custom menu is used #867

Merged
merged 2 commits into from
Jan 21, 2020

Conversation

jpkrohling
Copy link
Contributor

@jpkrohling jpkrohling commented Jan 20, 2020

Closes #644 by automatically adding a "Log Out" option when one seems to be missing. Note that this is only relevant when the OAuth Proxy is being used, which is currently the case only for OpenShift deployments.

Signed-off-by: Juraci Paixão Kröhling [email protected]

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling requested a review from jkandasa January 20, 2020 16:04
@jpkrohling
Copy link
Contributor Author

jpkrohling commented Jan 20, 2020

@jkandasa would you be able to give this one a try ? Image: quay.io/jpkroehling/jaeger-operator:644-Add-Logout . This should be the version reported by the operator: v1.16.0-16-g9bb95df3

@jkandasa
Copy link
Member

@jpkrohling I tested this operator(quay.io/jpkroehling/jaeger-operator:644-Add-Logout). I can see the Log Out option.

image

jaeger operator:

time="2020-01-21T10:04:29Z" level=info msg=Versions arch=amd64 identity=openshift-operators.jaeger-operator jaeger=1.16.0 jaeger-operator=v1.16.0-16-g9bb95df3 operator-sdk=v0.12.0 os=linux version=go1.13.4

CR Files used:

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaegerqe
  namespace: jkandasa
spec:
  strategy: allInOne
  allInOne:
    image: 'registry.redhat.io/distributed-tracing/jaeger-all-in-one-rhel7:1.13'
    options:
      log-level: debug
      query:
        base-path: /jaeger
  ui:
    options:
      dependencies:
        menuEnabled: false
      tracking:
        gaID: UA-000000-2
      menu:
        - label: About Jaeger
          items:
            - label: Documentation
              url: 'https://www.jaegertracing.io/docs/latest'
  storage:
    options:
      memory:
        max-traces: 100000

When I add the option (security: none) on CR, Log Out options is not displayed.

  ingress:
    security: none

Copy link
Member

@jkandasa jkandasa left a comment

Choose a reason for hiding this comment

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

LGTM

@jpkrohling
Copy link
Contributor Author

@pavolloffay would you be able to review this one?

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM.

I haven't tested if it works.

json.Unmarshal([]byte(menuStr), &menuArray)
logout := map[string]interface{}{
"label": "Log Out",
"url": "/oauth/sign_in",
Copy link
Member

Choose a reason for hiding this comment

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

sign_in for log out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :)


// SkipLogout tells the operator to not automatically add a "Log Out" menu option to the custom Jaeger configuration
// +optional
SkipLogout *bool `json:"skipLogout,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

does it need to be a pointer? We probably want the default to be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For all new properties, we probably want a tri-state value: nil (not set), true, false. This way, the property doesn't get persisted as part of the CR, and we can have different defaults depending on the platform.

@jpkrohling
Copy link
Contributor Author

As @jkandasa tested that it works as intended, I'm merging this.

@jpkrohling jpkrohling merged commit 643cb75 into jaegertracing:master Jan 21, 2020
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.

"LogOut" options missing in the default all-in-one cr config
3 participants