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: Acutally validate config struct #3837

Merged

Conversation

simonswine
Copy link
Contributor

We seem to have not linked up the validation for a few components.

@simonswine simonswine added the type/bug Something isn't working label Jan 14, 2025
@simonswine simonswine self-assigned this Jan 14, 2025
We seem to have not linked up the validation for a few components.
@simonswine simonswine force-pushed the 20250114_actually-validate-config branch from 6ab80d3 to b11bc15 Compare January 14, 2025 10:34
@simonswine simonswine marked this pull request as ready for review January 14, 2025 10:45
@simonswine simonswine requested a review from a team as a code owner January 14, 2025 10:45
Copy link
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@simonswine simonswine merged commit 68c28f5 into grafana:main Jan 14, 2025
18 checks passed
@kolesnikovae
Copy link
Collaborator

This might be a breaking change. Does it make sense to add a flag that enables/disables config validation?

@julienvey
Copy link

julienvey commented Feb 20, 2025

I think we hit this breaking change.

We were using a storage_prefix value for s3 storage (the bucket is shared for multiple application / envs)

storage_prefix: pyroscope/my-env/

and this is now failing on the latest versions of Pyroscope

failed creating pyroscope: storage prefix contains invalid characters, it may only contain digits and English alphabet letters

I see the code for regex validation has been there for more than 3 years

storage_prefix with a / has been working fine so far, I don't really see a reason why / or - should be excluded.

Is there any workaround for us to keep the current storage prefix ? Otherwise we would need to set up a new one without the /, and we would either loose all existing data, or would need to move it for all environments

shelldandy pushed a commit to shelldandy/pyroscope that referenced this pull request Mar 14, 2025
We seem to have not linked up the validation for a few components.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something isn't working
4 participants