Skip to content

⚠️ update name validation patterns to match Kubernetes as close as possible - #1175

Merged
joelanford merged 2 commits into
operator-framework:mainfrom
everettraven:chore/kubernetes-naming-validations
Sep 4, 2024
Merged

⚠️ update name validation patterns to match Kubernetes as close as possible#1175
joelanford merged 2 commits into
operator-framework:mainfrom
everettraven:chore/kubernetes-naming-validations

Conversation

@everettraven

Copy link
Copy Markdown
Contributor

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@everettraven
everettraven requested a review from a team as a code ownerAugust 26, 2024 20:59
@netlify

netlifyBot commented Aug 26, 2024

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commitb9797ec
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66d84fbdf2b6050009ef04fa
😎 Deploy Previewhttps://deploy-preview-1175--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov

codecovBot commented Aug 26, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.13%. Comparing base (5800c7f) to head (b9797ec).
Report is 2 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (5800c7f) and HEAD (b9797ec). Click for more details.

HEAD has 2 uploads less than BASE
FlagBASE (5800c7f)HEAD (b9797ec)
unit21
e2e21
Additional details and impacted files
@@ Coverage Diff @@## main #1175 +/- ##
===========================================
- Coverage 77.56% 56.13% -21.43% 
===========================================
Files 36 36 Lines 1979 1979 ===========================================
- Hits 1535 1111 -424 - Misses 309 781 +472 + Partials 135 87 -48 
FlagCoverage Δ
e2e0.00% <ø> (-57.66%)⬇️
unit56.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment threadapi/v1alpha1/clusterextension_types.go
Comment threadapi/v1alpha1/clusterextension_types.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do the before or after changes from this PR align to the validations that happen in opm validate? We need to make sure that package and channel names are fully specified and validated the exact same way in both places.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

As far as I can tell, the validation for the package name does match. There does not appear to be any validation on channel names in opm validate.

IMO we should merge the validation changes here and then add the validation of the channel name to opm validate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we need to check known catalogs against these proposed validations before we merge so that we have more information to make a decision. We may ultimately decide to implement these validations despite breaking existing content, but we should know before we act.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Gist that verified this validation works for all channel names in all known catalogs (except for one in the redhat certified operators index that looks like an unintentional channel name): https://gist.github.com/everettraven/4691c8cc6181f28b129f46cceeb602b5

@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2024
Comment threadinternal/controllers/clusterextension_admission_test.go
Comment threadapi/v1alpha1/clusterextension_types.go Outdated
@everettraven
everettravenforce-pushed the chore/kubernetes-naming-validations branch from c0bf806 to bfaf8cbCompareSeptember 3, 2024 23:12
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2024
…sible
Signed-off-by: everettraven <everettraven@gmail.com>
@everettraven
everettravenforce-pushed the chore/kubernetes-naming-validations branch from c629ded to 61417d9CompareSeptember 4, 2024 12:12
Comment threadinternal/controllers/clusterextension_admission_test.go Outdated
Signed-off-by: everettraven <everettraven@gmail.com>
@joelanford
joelanford added this pull request to the merge queue Sep 4, 2024
Merged via the queue into operator-framework:main with commit f797d50Sep 4, 2024
@skattojuskattoju mentioned this pull request Sep 25, 2024
4 tasks
Sign up for freeto 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.

Update validations for fields referencing the name of something to follow Kubernetes

3 participants

@everettraven@joelanford@openshift-merge-robot