-
Notifications
You must be signed in to change notification settings - Fork 61
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
Create route functionality added #4140
Create route functionality added #4140
Conversation
…eate-routes-from-the-extension
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4140 +/- ##
===========================================
+ Coverage 32.37% 44.20% +11.82%
===========================================
Files 85 90 +5
Lines 6505 7237 +732
Branches 1349 1526 +177
===========================================
+ Hits 2106 3199 +1093
+ Misses 4399 4038 -361 ☔ View full report in Codecov by Sentry. |
@msivasubramaniaan Could you please rebase the PR on top of the main branch? |
@vrubezhny I have done it already. Seems there were no more commits on main branch |
An annoying effect occurs on the Create route view when typing in something to the text fields... When trying to type in something into the middle of the existing text after every character is added it moves the text cursor to the end of the string, so you have to manually move the cursor back to the required position before typing in the next character: Screencast.from.2024-05-22.13-51-18.webm... I haven't spotted this on any other view we have. |
When I'm trying to create a route on Kind cluster it fails (probably with some reason, but I have created a similar route on Sandbox cluster with no any problem) : After that I cannot create any other route until I close the |
I don't think it's true, neither GH thinks it is: |
@vrubezhny FYR |
Hm. Interesting... 🤔 GH still shows me "This branch cannot be rebased due to conflicts" and I really had to rebase (locally) due to a conflict in "imports" part of |
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.
Just some extra lines to be removed here. Plus a few questions I asked in the main thread (field editing question, error on Kind cluster and recovery after a route creation error)
I've addressed all mentioned review comments. please have a look |
@msivasubramaniaan It still shows me that your branch is not rebased on top of the main branch and has a merge conflict in |
@msivasubramaniaan Now for Sandbox cluster I have: IMHO, the validator for Path field of |
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.
@msivasubramaniaan As routes is an Openshift specific objects - we should show it as well as Create Route
context menu item only when working with an Openshift cluster
If you are trying to expose a Deployment using a Route, you first need to create a Service for the Deployment. I had to do this from the command line ( It might be nice to incorporate creating the service if it doesn't already exist as a step in the route creation process. For instance, the user selects the deployment instead of the service, then when they submit the form, the extension checks if there is an existing service connect to the deployment and creates it if there isn't one, then it exposes the service. |
@datho7561 I will take this as improvement and create a separate ticket and work on it |
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.
Looks good to me. Thanks!
@msivasubramaniaan Don't forget to open a separate issue for creating the service if it doesn't already exist as a step in the route creation process
- I agree in that it would be a useful addition.
I am okay with handling creating a service for a deployment in a separate issue. We should squash the commits before merging, this means you will need to do a rebase on main @msivasubramaniaan . |
Fixes: #4086