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

versionPattern should be more flexible to allow internal versions #3739

Open
robzienert opened this issue Jan 24, 2024 · 4 comments · Fixed by #3742 · May be fixed by #3986
Open

versionPattern should be more flexible to allow internal versions #3739

robzienert opened this issue Jan 24, 2024 · 4 comments · Fixed by #3742 · May be fixed by #3986
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@robzienert
Copy link
Contributor

robzienert commented Jan 24, 2024

What broke? What's expected?

The versionPattern enforced by kubebuilder requires all CRD versions to start with a v, however this makes usage of an internal version confusing (e.g. creating a v0) where an internal or __internal version would communicate intent more clearly; __internal is even used within the core Kubernetes codebase.

What I'd like to see is the versionPattern relaxed to make an allowance for internal versions, something like so:

Before:

const (
	versionPattern = "^v\\d+(?:alpha\\d+|beta\\d+)?$"
)

After:

const (
	versionPattern = "^(?:__internal|v\\d+(?:alpha\\d+|beta\\d+)?)$"
)

Doing so would be additive and not break any of the existing uses.

Reproducing this issue

No response

KubeBuilder (CLI) Version

v3.12.0

PROJECT version

No response

Plugin versions

No response

Other versions

No response

Extra Labels

No response

@robzienert robzienert added the kind/bug Categorizes issue or PR as related to a bug. label Jan 24, 2024
@camilamacedo86
Copy link
Member

camilamacedo86 commented Jan 25, 2024

Hi @robzienert

Thank you for raising this one.
We should do the same k8s validation.

In this case, it might to be (Required to check): func IsDNS1035Label(value string) []string {

However, we need to TBD the right k8s validation and use the same.
Feel free to check it out and open a PR with the proposed solution.

Your help is very welcome.

k8s-ci-robot added a commit that referenced this issue Jan 30, 2024
🐛 Align CRD version validation with apiextensions
@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 16, 2024

Hi @robzienert

My sincere apologies, I oversight this one.
The PR #3742 introduced an regression.

In Kubernetes Custom Resource Definitions (CRDs), Kind names must not include spaces, special characters (e.g., @, #, $, %, &, *), or punctuation marks. They should strictly adhere to CamelCase format, starting with an uppercase letter and not violating the general programming identifier rules (e.g., no starting with numbers, no reserved words). This ensures compatibility with Kubernetes API and client code generation tools.

The func IsDNS1035Label(value string) []string { is NOT accurated for this case.

The correct one seems to be (from the same file):

// IsDNS1123Subdomain tests for a string that conforms to the definition of a
// subdomain in DNS (RFC 1123).
func IsDNS1123Subdomain(value string) []string {
	return dns1123SubdomainConfig.check(value)
}

Therefore, to close this one we need change the validation like was done in the PR #3742

However, to avoid we introduce and issue we should add more tests in the project to ensure that names like DNS will be allowed to be used.

@camilamacedo86 camilamacedo86 added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Feb 16, 2024
@Sajiyah-Salat
Copy link
Contributor

@camilamacedo86 is this really a good first issue? I dont see the clear cut implementation to close this issue.

@Kavinjsir Kavinjsir linked a pull request Jun 14, 2024 that will close this issue
@Kavinjsir
Copy link
Contributor

@camilamacedo86 is this really a good first issue? I dont see the clear cut implementation to close this issue.

@Sajiyah-Salat The code for implementation isn’t extensive, which makes it a good-first-issue.
However, since it’s marked as priority/important-soon, I've gone ahead and created a PR to progress: #3986.
If you're still interested, would you be willing to help with the review? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
4 participants