Resourcenamelimit 63chars #1519
Conversation
tesshuflower
force-pushed
the
resourcenamelimit_63chars
branch
from
fe29fe5 to
76fd79e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1519 +/- ##
=======================================
+ Coverage 66.3% 66.4% +0.1%
=======================================
Files 57 57
Lines 7474 7501 +27
=======================================
+ Hits 4959 4986 +27
Misses 2230 2230
Partials 285 285
|
|
|
/cc @JohnStrunk |
| // serviceName is the name of the service to connect to for incoming SSH | ||
| // replication connections. | ||
| //+optional | ||
| ServiceName *string `json:"serviceName,omitempty"` |
There was a problem hiding this comment.
I'd like to hear more about why this is needed and whether it is applicable to any use-cases other than ODF.
There was a problem hiding this comment.
@JohnStrunk I think the main use-case is for ODF to be honest. The reason is they've been relying on knowing what the service name will be, based on the CR name. Their use case involves needing to create a servicexport CR for submariner, so that the service will be accessible from the remote cluster (as if it was a local service). I'm not sure if others will have something similar.
There was a problem hiding this comment.
Do you have a concern over exposing the service John? I guess I thought if ODF has this then others would as well eventually. Things like submariner that expose a service across clusters, or even for users that want to access a service by name like <svcname>.<namespace>.svc.cluster.local....but maybe this isn't as common unless they're working in the same cluster.
There was a problem hiding this comment.
There's 2 things really.
The 1st is that I'm not interested in special cases/behaviors for specific apps, even if it's one of ours because that means that one of the two apps is probably not doing something "correctly".
But the 2nd (related) item is that this raises the question of whether we're not doing something "correctly"... i.e., not in a general/best-practices way to work with other pieces of infrastructure. In reviewing this PR, it occurred to be that we probably should have just used labels/selectors to track the jobs, etc. and let kube pick the names via generateName --- not suggesting we change it, just an observation. But let's say someone is using Istio, Submariner, or something else... How should we be be exposing things? Should be be adding labels instead of providing the Service name in a custom field?
- use crc32 hash of CR name for names that are too long - new e2e tests for long names - e2e tests in customscorecard-tests - rearrange for concurrency Signed-off-by: Tesshu Flower <[email protected]>
tesshuflower
force-pushed
the
resourcenamelimit_63chars
branch
from
2b9fd41 to
f2b0978
Compare
|
/retest |
|
/cc @JohnStrunk I've made updates as we discussed - can you take a look when you get a chance? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JohnStrunk, tesshuflower The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Describe what this PR does
Is there anything that requires special attention?
Related issues:
Fixes: #1470