Skip to content

feat: added name validation scheme as a config field and flag as well #6733

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

av153k
Copy link

@av153k av153k commented May 8, 2025

What this PR does:

  • Adds name_validation_scheme as yaml flag and name.validation.scheme as a flag
  • Remove config init and set the model.NameValidationScheme directly in New() method to creating a Cortex object

Which issue(s) this PR fixes:
Fixes #6702

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@av153k av153k force-pushed the feat/namevalidationscheme-flag branch from 2186312 to 146188f Compare May 8, 2025 07:00
@av153k av153k marked this pull request as ready for review May 8, 2025 09:54
@av153k
Copy link
Author

av153k commented May 8, 2025

I am working on the tests for this.

@av153k
Copy link
Author

av153k commented May 10, 2025

Added test for name validation scheme in TestConfigValidation in cortex_test.go

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks @av153k. Great work and I think it is on the right track. Just few comments

…andled empty name validation scheme and related tests

Signed-off-by: Abhishek Anand <[email protected]>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Can you also update changelog?

@av153k
Copy link
Author

av153k commented May 11, 2025

Can you also update changelog?

What would be the version of this change, though?

… in the config file and CLI flag, along with corresponding tests.

Signed-off-by: Abhishek Anand <[email protected]>
case prom_config.UTF8ValidationConfig:
model.NameValidationScheme = model.UTF8Validation
default:
fmt.Errorf("invalid name validation scheme")
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be returned

Copy link
Author

@av153k av153k May 13, 2025

Choose a reason for hiding this comment

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

Okay. I will add that. Anything else apart from this? Also after this is done, can you please point me to a few issues that you think I can take up on. Thank you.

@av153k av153k changed the title Added name validation scheme as a config field and flag as well feat: added name validation scheme as a config field and flag as well May 13, 2025
@av153k
Copy link
Author

av153k commented May 13, 2025

There was some issue with the generated doc I guess. Will switch to WSL since I am using windows and then push one more commit with generated docs.

@yeya24
Copy link
Contributor

yeya24 commented May 13, 2025

Please fix lint and unit test. You can also run those tests locally and see if they are able to pass

@av153k
Copy link
Author

av153k commented May 14, 2025

Please fix lint and unit test. You can also run those tests locally and see if they are able to pass

I had to add back the validation check in the config.validate() for name validation scheme. I though about removing that specific test but felt its much better to add the validation logic.

I am working on the lint issues.

@av153k
Copy link
Author

av153k commented May 14, 2025

@yeya24 Do I have to add the test for race cases or will that be in a separate PR?

@av153k
Copy link
Author

av153k commented May 14, 2025

I am trying make doc to generate the docs and fix the lint issue but I am getting hit with some qemu arch related error. Any help? @yeya24

Actually now, just stuck here.

image

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

Successfully merging this pull request may close these issues.

Make NameValidationScheme configurable
2 participants