Edit Knative domain after it has been deployed
What does this MR do?
This MR adds the ability to edit Knative domain after it has been deployed.
What are the relevant issue numbers?
Closes #56937 (closed)
Successful update
Update failure
To simulate an "Update failure", the following commands can be executed:
Open rails console: bundle exec rails console
In rails console execute the following:
-
cluster = Clusters::Cluster.find <id>
Note:<id>
can be found in the browser url when navigating to the cluster knative = cluster.application_knative
knative.update!(status: 6)
The runner on the specified cluster should now be in the update_errored
state, and should display the Retry upgrade
button
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides
Merge request reports
Activity
changed milestone to %11.9
added Category:Kubernetes Management Deliverable Persona: DevOps Engineer Persona: Software developer backend devopsconfigure [DEPRECATED] frontend ~2975006 + 1 deleted label
4 Warnings This merge request is quite big (more than 1258 lines changed), please consider splitting it into multiple merge requests. 3bdff7aa: This commit’s subject line is acceptable, but please try to reduce it to 50 characters. f8234d9a: This commit’s subject line is acceptable, but please try to reduce it to 50 characters. This merge request changed files with disabled eslint rules. Please consider fixing them. Disabled eslint rules
The following files have disabled
eslint
rules. Please consider fixing them:app/assets/javascripts/clusters/components/application_row.vue
Run the following command for more details
node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \ 'app/assets/javascripts/clusters/components/application_row.vue'
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer frontend Luke Bennett ( @lbennett
)Phil Hughes ( @iamphill
)backend Rubén Dávila ( @rdavila
)Douwe Maan ( @DouweM
)Generated by
DangerEdited by 🤖 GitLab Bot 🤖mentioned in issue #56937 (closed)
mentioned in issue gitlab-design#271 (closed)
- Resolved by Jacques Erasmus
added 1 commit
- ecdbe9b4 - Remove bootstrap column + minor UX adjustments
added 173 commits
Toggle commit listmarked the checklist item Changelog entry added, if necessary as completed
@tauriedavis this MR still needs some more work in terms of functionality, but I think it's ready for some early UX feedback.
Could you please have a look and let me know if there's anything that does not look right?
Note the
Save changes
button does not work yet since we still need the backend integrationassigned to @tauriedavis
- Resolved by Jacques Erasmus
assigned to @jerasmus
Happy to test the UX once the backend is in.
@tauriedavis I don't think you need to wait. It should all work now. The only backend changes are just about ensuring we don't do an upgrade of Knative if the version changes. Since your tests won't be using an old Knative install that could be upgraded you won't notice any difference after the backend changes are made. They may get merged independently (before or after) this MR I believe.
@DylanGriffith I don't think it works yet
@jerasmus also confirmed here that the save button isn't functional yet. https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25386#note_143892870
Edited by Taurie Davisjerasmus also confirmed here that the save button isn't functional yet. https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25386#note_143892870
@jerasmus could you confirm if you are waiting on backend changes here or just to write the frontend code that calls the backend? I assumed that you would just call the upgrade API for now since that should work.
@DylanGriffith since the backend sets the status to
updating
/update_errored
once the upgrade API is called, the frontend will respond as if an upgrade is happening as you can see here https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25386#note_144176504So I don't think we'll get away with using the upgrade API as a first iteration (unless you have some suggestions..?) I've been speaking to @Alexand and he seemed to have a grasp of how we'll go about implementing this on the backend.
- Resolved by Jacques Erasmus
From slack:
From what I am seeing I think it is not making the problem any worse today if we just make the updating of hostname use the same upgrade logic with the same error states since we aren't even really using those error states properly anyway. And this will allow us to actually ship this thing quite quickly and it's a valuable feature.
Based on the fact that the error messages don't actually say anything about what is wrong (eg. is the app even running?) I think we won't actually be making things any worse if we just always say "Failed" regardless of update or upgrade. Since the situation is still the same. They may be able to click upgrade or update again and it may fix this or it may not.
Is there a reason why the changes would not succesfully save? Shouldn't we be using a validation? My concern is that currently, if there is a failure, it seems like the user is stuck in an inbetween state where there is no longer an IP address and it doesn't appear they can use the previous domain they entered. Ideally, we would show a validation error if something fails, which would not remove the IP address. Essentially, nothing would be saved and the old domain would still be present.
added 203 commits
-
dec3eece...fb76dfe0 - 199 commits from branch
master
- 47c7ebd7 - Add ability to edit Knative domain
- c0a40fb6 - Generate new translations
- ef594135 - Update copy for Knative
- fc05fa07 - Generate new translations
Toggle commit list-
dec3eece...fb76dfe0 - 199 commits from branch
added 1 commit
- 877b3f23 - Display IP address when Knative upgrade fails
@tauriedavis I've made some changes in order to make the process of saving the domain using the
upgrade
request a little more user-friendly. Unfortunately, you'll still see theupgrading
spinner when updating the domain and theupgrade failure
(previews in description) message when the request failed. But as @DylanGriffith mentioned in his comment the way we currently do error handling isn't ideal anyway and this will at least allow us to ship this feature quickly.To simulate a failure:
Open rails console:
bundle exec rails console
In rails console execute the following:
-
cluster = Clusters::Cluster.find <id>
Note:<id>
can be found in the browser url when navigating to the cluster knative = cluster.application_knative
knative.update!(status: 6)
The runner on the specified cluster should now be in the
update_errored
state, and should display the error message with aRetry upgrade
buttonCould you please have another look at the current UX and let me know what you think?
-
assigned to @tauriedavis
marked the checklist item Tests added for this feature/bug as completed
marked the checklist item Tested in all supported browsers as completed
marked the checklist item Conforms to the code review guidelines as completed
marked the checklist item Conforms to the merge request performance guidelines as completed
marked the checklist item Conforms to the style guides as completed
@jerasmus the length of the domain name field compared to the IP address is too long. Can we shorten that so that we can give IP address a bit more room? I wonder if an address such as
123.123.123.123
will still show up correctly on the allotted space. /cc @tauriedavisAlso, the existing error message is
Something went wrong when upgrading Knative. Please check the logs and try again.
Can we update this error message to be a bit more generic? We'll follow-up with a dedicated error message for this scenario on https://gitlab.com/gitlab-org/gitlab-ce/issues/58230. For now we can simply display:Update failed. Please check the logs and try again
Lastly, can we update the
Knative IP Address
field to either to simplyIP Address
? (I consideredexternal IP address
but it's too long).Edited by Daniel Gruesso- Resolved by Daniel Gruesso
I wonder if an address such as
123.123.123.123
will still show up correctly on the allotted spaceThose characters fit just fine.
Would a Knative IP ever be longer?
However, I just though that since we are allowing hostnames in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25181 it would make sense to make the field longer since hostnames are often longer. Can we split the fields just 50/50 then @jerasmus?
Edited by Taurie Davis
- Resolved by Taurie Davis
I noticed when installing Knative, the orange warning about the IP being generated is above the help text. I think it makes more sense to keep the help text with the inputs and move the warning message below it. What do you think?