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

fix: return correct message about unsupported protocol #796

Merged
merged 3 commits into from Jul 20, 2022

Conversation

weisdd
Copy link
Contributor

@weisdd weisdd commented Jul 20, 2022

Description

There's a cosmetic bug in getGrafanaAdminUrl - if unsupported protocol is chosen through config.server.protocol, the operator would always complain about http instead of actual protocol: server protocol http is not supported, please use either http or https.

The PR fixes that.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

Verification steps

Deploy grafana with the following config:

apiVersion: integreatly.org/v1alpha1
kind: Grafana
metadata:
  name: example-grafana
spec:
  client:
    preferService: true
  config:
    server:
      protocol: unknown-protocol

You should see a reference to unknown-protocol in the error message:
grafana-operator-846d554b57-vvxgs grafana-operator 2022-07-20T16:28:21.238Z DEBUG controller-runtime.manager.events Warning {"object": {"kind":"Grafana","namespace":"monitoring","name":"example-grafana","uid":"f218c504-fd0a-4fc2-8e6f-5519f369c97c","apiVersion":"integreatly.org/v1alpha1","resourceVersion":"503482"}, "reason": "ProcessingError", "message": "server protocol unknown-protocol is not supported, please use either http or https"}

weisdd added 2 commits Jul 20, 2022
Signed-off-by: Igor Beliakov <demtis.register@gmail.com>
Signed-off-by: Igor Beliakov <demtis.register@gmail.com>
@weisdd weisdd force-pushed the fix/server-protocol-error branch from 24e808b to 3dd7eb6 Compare Jul 20, 2022
@weisdd
Copy link
Contributor Author

weisdd commented Jul 20, 2022

@HubertStefanski since gofumpt automatically made the same code style changes as in the branch that got merged to master, there was a merge conflict. I've just rebased the branch to mitigate that, everything is fine now.

@NissesSenap
Copy link
Member

NissesSenap commented Jul 20, 2022

@weisdd why not use a enum In the CRD to solve this as well? We can of course still update code as well.

@weisdd
Copy link
Contributor Author

weisdd commented Jul 20, 2022

@NissesSenap it's actually already covered by the enum that was introduced in the branch with probes refactoring. If we are to assume that all people keep their CRDs updated, then we can remove this part of the code altogether. If we rather decide to keep some sort of additional config validation through the code for v4.x, then we should better leave it for now and maybe deprecate in v5. WDYT? :)

@NissesSenap
Copy link
Member

NissesSenap commented Jul 20, 2022

Sounds very reasonable. Always good to print out extra info.

@NissesSenap NissesSenap merged commit ce14a5a into grafana-operator:master Jul 20, 2022
8 checks passed
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.

None yet

2 participants