Skip to content
Snippets Groups Projects

Edit Knative domain after it has been deployed

Merged Jacques Erasmus requested to merge 56937-edit-knative-domain into master

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

rec1.2019-02-27_11_11_08

Update failure

rec2.2019-02-27_11_12_26

To simulate an "Update failure", the following commands can be executed:

Open rails console: bundle exec rails console

In rails console execute the following:

  1. cluster = Clusters::Cluster.find <id> Note: <id> can be found in the browser url when navigating to the cluster
  2. knative = cluster.application_knative
  3. 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?

Edited by João Alexandre Cunha

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • ecdbe9b4 - Remove bootstrap column + minor UX adjustments

    Compare with previous version

  • Jacques Erasmus resolved all discussions

    resolved all discussions

  • Jacques Erasmus added 173 commits

    added 173 commits

    Compare with previous version

  • Jacques Erasmus marked the checklist item Changelog entry added, if necessary as completed

    marked the checklist item Changelog entry added, if necessary as completed

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • a0bee716 - Add ability to edit Knative domain

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • @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 integration

  • UI looks good! One small copy comment.

    Happy to test the UX once the backend is in.

  • 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

    Screen_Shot_2019-02-22_at_11.40.55_AM

    Screen_Shot_2019-02-22_at_11.44.02_AM

    @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 Davis
  • jerasmus 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.

  • added 1 commit

    Compare with previous version

  • Jacques Erasmus resolved all discussions

    resolved all discussions

  • @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_144176504

    So 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.

  • added 1 commit

    Compare with previous version

    • 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.

  • Jacques Erasmus added 203 commits

    added 203 commits

    Compare with previous version

  • added 1 commit

    • 877b3f23 - Display IP address when Knative upgrade fails

    Compare with previous version

  • Jacques Erasmus changed the description

    changed the description

  • added 1 commit

    • 1a0e3251 - Pretty print changed frontend files

    Compare with previous version

  • added 1 commit

    • 8509841f - Add ability to edit Knative domain

    Compare with previous version

  • Jacques Erasmus changed the description

    changed the description

  • @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 the upgrading spinner when updating the domain and the upgrade 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:

    1. cluster = Clusters::Cluster.find <id> Note: <id> can be found in the browser url when navigating to the cluster
    2. knative = cluster.application_knative
    3. 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 a Retry upgrade button

    Could you please have another look at the current UX and let me know what you think?

  • Jacques Erasmus marked the checklist item Tests added for this feature/bug as completed

    marked the checklist item Tests added for this feature/bug as completed

  • Jacques Erasmus marked the checklist item Tested in all supported browsers as completed

    marked the checklist item Tested in all supported browsers as completed

  • added 1 commit

    Compare with previous version

  • Jacques Erasmus marked the checklist item Conforms to the code review guidelines as completed

    marked the checklist item Conforms to the code review guidelines as completed

  • Jacques Erasmus marked the checklist item Conforms to the merge request performance guidelines as completed

    marked the checklist item Conforms to the merge request performance guidelines as completed

  • Jacques Erasmus marked the checklist item Conforms to the style guides 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 @tauriedavis

    Also, 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 simply IP Address? (I considered external IP address but it's too long).

    Edited by Daniel Gruesso
    • Resolved by Taurie Davis

      @jerasmus

      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?

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading