Skip to content
This repository has been archived by the owner on Aug 12, 2024. It is now read-only.

use stable hash for labels and name suffixes (aligning with OLMv0 changes) #764

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

joelanford
Copy link
Member

@joelanford joelanford requested a review from a team as a code owner November 17, 2023 14:48
internal/convert/registryv1.go Outdated Show resolved Hide resolved
internal/convert/registryv1.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (4f98bdd) 21.14% compared to head (0b87aa6) 20.62%.

Files Patch % Lines
internal/util/util.go 9.09% 19 Missing and 1 partial ⚠️
internal/convert/registryv1.go 0.00% 10 Missing ⚠️
internal/util/hash.go 77.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #764      +/-   ##
==========================================
- Coverage   21.14%   20.62%   -0.52%     
==========================================
  Files          14       14              
  Lines        1069     1086      +17     
==========================================
- Hits          226      224       -2     
- Misses        795      812      +17     
- Partials       48       50       +2     

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

stevekuznetsov
stevekuznetsov previously approved these changes Nov 17, 2023
@stevekuznetsov
Copy link
Member

@joelanford e2e looks very borked

@joelanford
Copy link
Member Author

joelanford commented Nov 28, 2023

Looks like the bundle validation is rejecting the newly generated bundle name.

Seems like this is a built-in validation for most resource types...
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names

Bundle.core.rukpak.io "ahoy-v8vq7-6L2Nq4N6UYLYpmq6ffZkPgbGnbi6mxZCYug8PO" is invalid: metadata.name: Invalid value: "ahoy-v8vq7-6L2Nq4N6UYLYpmq6ffZkPgbGnbi6mxZCYug8PO": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, ''-'' or ''.'', and must start and end with an alphanumeric character (e.g. ''example.com'', regex used for validation is ''a-z0-9?(.a-z0-9?)*'')'

@stevekuznetsov
Copy link
Member

That error might be misleading, I think you pass the regex fine, but may be running afoul of the length requirement. Suggest not adding the prefixes unless you really need to?

@joelanford
Copy link
Member Author

It definitely looks like the regex rejects uppercase letters. I changed to base36 and now seeing the length validation failure.

Bundle.core.rukpak.io "cincinnatiz6xhc-34402ftwebb3yugoq821w197i1988guvd9k6oqjcgm92" is invalid: metadata.name: Too long: may not be longer than 52'

@joelanford
Copy link
Member Author

joelanford commented Nov 28, 2023

I'm kinda leaning toward base36 + truncate the base name, kinda like we do with RBAC. But that leaves only 8 characters for the base name if we don't also truncate the hash.

🤔

@joelanford
Copy link
Member Author

length of 52 is coming from here:

I think that was because we knew that we wanted to leave some extra characters available for other objects that might need to exist for the bundle (right now, that's at least the unpack pod, can't remember if there are others)

@stevekuznetsov stevekuznetsov added this pull request to the merge queue Nov 28, 2023
Merged via the queue into operator-framework:main with commit b4e9976 Nov 28, 2023
10 of 11 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants