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

Output a warning when manageTLS is enabled but the port is 9080 #474

Open
4 tasks done
leochr opened this issue May 2, 2023 · 5 comments
Open
4 tasks done

Output a warning when manageTLS is enabled but the port is 9080 #474

leochr opened this issue May 2, 2023 · 5 comments
Assignees

Comments

@leochr
Copy link
Member

leochr commented May 2, 2023

As part of the validation of CR, output a warning (not error) when manageTLS is enabled (by default it is) but .spec.service.port is set to 9080. The message should be output to Reconciled status

  • Completed for RCO
  • Completed for OLO
  • Completed for WLO
  • Tests
@idlewis idlewis self-assigned this Jul 7, 2023
@idlewis
Copy link
Member

idlewis commented Jul 10, 2023

@leochr Is the intention here to add a message field to the type: Reconciled status condition?

status:
  conditions:
  - lastTransitionTime: "2023-07-10T09:33:46Z"
    status: "True"
    type: Reconciled
  - lastTransitionTime: "2023-07-10T09:33:49Z"
    message: Application is reconciled and resources are ready.
    status: "True"
    type: Ready
  - lastTransitionTime: "2023-07-10T09:33:49Z"
    message: 'Deployment replicas ready: 1/1'
    reason: MinimumReplicasAvailable
    status: "True"
    type: ResourcesReady
  imageReference: registry.k8s.io/pause:3.2
  references:
    saResourceVersion: "1365018"
    svcCertSecretName: example-runtime-component-svc-tls-ocp
  versions:
    reconciled: 1.2.1

@leochr
Copy link
Member Author

leochr commented Jul 10, 2023

@idlewis that's right

@leochr
Copy link
Member Author

leochr commented Jul 11, 2023

@idlewis Just thought about the visibility to users. The user may not notice the warning if it's inside the Reconciled message - especially when its status condition is 'True'. Adding a new StatusCondition named Warning that includes the warning message(s) would be better. Set the status to 'True' when there is a warning. Omit the StatusCondition or set it to 'False' otherwise.

@idlewis
Copy link
Member

idlewis commented Jul 11, 2023

@leochr The operator currently only seems to set status conditions from inside ManageError and ManageSuccess. Would those two places stil be the correct place to set this new status condition?

@leochr
Copy link
Member Author

leochr commented Jul 11, 2023

@idlewis Yes, I believe so

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

No branches or pull requests

2 participants