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

Added basic status to CR{D} #802

Merged

Conversation

jpkrohling
Copy link
Contributor

@jpkrohling jpkrohling commented Dec 3, 2019

This PR adds a new JaegerPhase type, added to the JaegerStatus. When running kubectl get jaegers, this is what is shown now:

$ kubectl get jaegers
NAME                         STATUS    VERSION
jaeger-all-in-one-inmemory   Running   1.15.1

When getting the output as YAML for a specific instance, here's the output:

$ kubectl get jaegers jaeger-all-in-one-inmemory -o yaml | tail
      resources: {}
      schedule: '*/30 * * * *'
    options: {}
    type: memory
  strategy: allinone
  ui:
    options: {}
status:
  phase: Running
  version: 1.15.1

And here's how it shows up in OLM:

image

Note that the Version field isn't being showed in OLM yet.

This is related to #259, although it won't close it, as we might want to keep that open to discuss further status fields in the future.

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

@jpkrohling
Copy link
Contributor Author

According to people on the #forum-operator-fw in the CoreOS slack, the Version column in the UI shows the value of .Spec.Version. Note that we have a .Status.Version, not .Spec. I'm still trying to confirm what this field refers to, as we do not want the version field to be part of the spec (as it's managed by the operator, not defined by the user).

@jpkrohling jpkrohling changed the title Added basic status to CR{D} WIP - Added basic status to CR{D} Dec 4, 2019
@jpkrohling jpkrohling force-pushed the 259-Add-basic-status-to-CR branch from b690e2a to 5067478 Compare December 4, 2019 11:37
@jpkrohling jpkrohling changed the title WIP - Added basic status to CR{D} Added basic status to CR{D} Dec 4, 2019
@jpkrohling
Copy link
Contributor Author

This PR is now ready to be reviewed. There's still one thing that could be better: when creating a CR, it would be good to have the initial state as "Pending". But to do that, we have to persist the status and finish the reconciliation loop, as this change itself will trigger an update. I decided to not add the Pending state, and have only a Failed or Running.

@jpkrohling jpkrohling requested a review from objectiser December 4, 2019 11:39
deploy/crds/jaegertracing.io_jaegers_crd.yaml Outdated Show resolved Hide resolved
pkg/apis/jaegertracing/v1/jaeger_types.go Outdated Show resolved Hide resolved
pkg/apis/jaegertracing/v1/jaeger_types.go Outdated Show resolved Hide resolved
pkg/controller/jaeger/jaeger_controller.go Outdated Show resolved Hide resolved
pkg/controller/jaeger/jaeger_controller_test.go Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Contributor Author

PR updated.

@jpkrohling jpkrohling force-pushed the 259-Add-basic-status-to-CR branch 3 times, most recently from 0890535 to 29e442c Compare December 5, 2019 13:41
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling force-pushed the 259-Add-basic-status-to-CR branch from 29e442c to 8c5197c Compare December 5, 2019 13:43
Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants