-
Notifications
You must be signed in to change notification settings - Fork 166
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
support static prefix for k8s resource name #3447
support static prefix for k8s resource name #3447
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3447 +/- ##
==========================================
+ Coverage 85.63% 85.66% +0.03%
==========================================
Files 1346 1347 +1
Lines 30586 30657 +71
Branches 8496 8529 +33
==========================================
+ Hits 26191 26261 +70
- Misses 4395 4396 +1
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment -- for consistency...
Testing this on cluster.
frontend/src/concepts/k8s/utils.ts
Outdated
): [string, AdditionalCriteriaApplied] => { | ||
const appliedCriteria: AdditionalCriteriaApplied = { | ||
autoGenerated: false, | ||
safeK8sPrefix: false, | ||
maxLength: false, | ||
isStaticPrefix: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isStaticPrefix: false, | |
staticPrefix: false, |
Maybe the wording here is weird -- they are all booleans, because they are status of what happened... appliedCriteria.staticPrefix
imo is better than appliedCriteria.isStaticPrefix
-- it's all the applied criteria so everything is prefixed with is
or none of them are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christianvogt this is not a blocker to me... but are you in the middle of doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this. I agree with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new commit with just the rename of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hah, a wee larger of a fix than I was asking... but okies
/approve Works as expected on cluster 👍 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne 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 |
/lgtm |
https://issues.redhat.com/browse/RHOAIENG-15509
Description
Adds a
staticPrefix
option to theK8sNameDescriptionField
component.When this is used in conjunction with
safePrefix
, the prefix will always be applied.Updated connection types form to use
staticPrefix
along with asafePrefix
ofct-
.When the user types the expected prefix, the prefix isn't duplicated:
Similarly, if the prefix is omitted by the user, the prefix is applied:
The user can change still customize the resource name.
When the name is left blank:
How Has This Been Tested?
ct-
Test Impact
Updated tests
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
cc @andrewballantyne @simrandhaliw