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

Change the namespace validation for oracle metrics #471

Open
wants to merge 1 commit into
base: 3.8.x
Choose a base branch
from

Conversation

andriy-dmytruk
Copy link
Contributor

@andriy-dmytruk andriy-dmytruk commented Jan 12, 2023

Add validation that namespace doesn't start with oci_

@andriy-dmytruk andriy-dmytruk marked this pull request as ready for review January 12, 2023 23:21
Copy link
Member

@alvarosanchez alvarosanchez 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 add some tests for the regex, please?

@andriy-dmytruk
Copy link
Contributor Author

andriy-dmytruk commented Jan 13, 2023

For some reason a namespace with a dash, like micronaut-test produced a 404 error - "invalid parameter, namespace must match [a-z][a-z0-9_]*[a-z0-9]". On the other hand, if I set the namespace to micronaut_test I always get a 400 saying namespace could not be registered.

This is weird as namespace matching ^[A-Za-z][A-Za-z0-9\\-]*[A-Za-z0-9]$ is used in PostMetricData examples.

I changed the pattern to ^[a-z][a-z0-9]*[a-z0-9]$ as it is the only one not producing any errors. But it seems strange, as there is no way to separate words now. @alvarosanchez Do you think an issue should be created to oci sdk, as this doesn't seem correct?

@alvarosanchez
Copy link
Member

Do you think an issue should be created to oci sdk, as this doesn't seem correct?

Absolutely. Please go ahead

@andriy-dmytruk
Copy link
Contributor Author

Seems like everything was correct in the first place. The oci sdk docs show an incorrect example of namespace, though.
The issue that I had was that I started the namespace with oci_... 😵‍ Added validation to verify that it doesn't.

@alvarosanchez
Copy link
Member

One test failed, can you please have a look?

Also, could you please create a remote branch in this repo instead of working on a fork? If you fork, there are some GitHub actions that will fail because the secrets are not exposed to forks.

@graemerocher
Copy link
Contributor

@andriy-dmytruk is this still needed?

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.

3 participants