Skip to content
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

Improve secret hostname hint #6873

Merged
merged 1 commit into from Dec 9, 2020

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Oct 8, 2020

Fixes:
https://issues.redhat.com/browse/ODC-4906

Analysis / Root cause:
When starting a pipeline there is an option to add a Secret to fetch data from a private git repository. The hint here suggest a server name like "https://github.com", but the pipeline/secret combination works only when the server name is something like "github.com".

Solution Description:
Update the hint from static The base server url (e.g. https://github.com) to a dynamic version which is now depending on the selected auth mechanism:

For docker config json: The base server url (e.g. https://quay.io/) (new example server)
For http basic auth: The base server url (e.g. https://github.com) (unchanged)
For SSH: Server hostname without schema or path (e.g. github.com)

Edit: After getting some feedback in this PR, this PR now also changes the order of the input fields to show depending fields below the more generic options.

It also hides some useless "Access To" and "Authentication Type" combinations:

  • For "Access To: Git" we only show the auth types "Basic Authentication" and "SSH Key" and dropped the option "Image Registry Credentials"
  • For "Access To: Image" we only show the auth types "Basic Authentication" and "Image Registry Credentials" and dropeed the option "SSH Key".

See #6873 (comment) and #6873 (comment)

Screen shots / Gifs for design review:
@openshift/team-devconsole-ux

Before:
old-layout

After:
updated-pr

And some more detail screens:

Access to options in now above the auth type
Docker registry auth type options:
Docker registry with basic auth selected:
Docker registry with Image Registry selected:
Git server auth type options:
Git server with basic auth selected:
Git server with SSH Key selected:

Unit test coverage report:
No unit test changed.

Test setup:

  • Install OpenShift Pipelines Operator
  • Create a new pipeline (e.g. add a deployment and enable the option "Add Pipeline")
  • Open the "Start Pipeline" dialog

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@jerolimov
Copy link
Member Author

/kind bug
/cc @karthikjeeyar

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 8, 2020
@karthikjeeyar
Copy link
Contributor

karthikjeeyar commented Oct 12, 2020

@jerolimov , The Server URL annotation's value can be https://github.com (for: https://github.com/test/testRepo) or github.com (for ssh url: git@github.com/test/testRepo), It depends on the git url that we use in the pipelines.
Refer the basic Auth and ssh examples here : https://github.com/tektoncd/pipeline/blob/master/docs/auth.md

@jerolimov jerolimov changed the title Improve secret hostname hint [WIP] Improve secret hostname hint Oct 21, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2020
@jerolimov jerolimov changed the title [WIP] Improve secret hostname hint Improve secret hostname hint Oct 27, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2020
@jerolimov
Copy link
Member Author

@karthikjeeyar You're absolute right here. We need to make a difference here between http and ssh. I updated this PR (incl the PR description and gif). Wdyt about this change now?

(I didn't tested the dockerconfig way and are not sure if https:// is correct in that case.)

@karthikjeeyar
Copy link
Contributor

@jerolimov Thank you for improving the hint based on the Authentication type, this is very useful to the user, but however i have a suggestion to reorder the Authentication Type and the Annotation section, because i feel changing the previous field's help text will not help the user to identify the value upfront, they might have to revisit the previous field to adjust the value. I think we need UX input here.
Auth_Type_reorder

@bgliwa01 Let us know your thought on this.

@jerolimov
Copy link
Member Author

jerolimov commented Oct 28, 2020

@karthikjeeyar Yes, I see this exactly like you. From the code perspective it looks a little bit strange, but I think also this would improve the UX. I reordered the components to get a better feeling for the result:

image
(Not part of this PR yet!)

@karthikjeeyar @bgliwa01 Wdyt? I removed the subtitle "Designate provider to be authenticated" because I was unsure where we want keep it. But I can restore it again if you want this.

@jerolimov
Copy link
Member Author

jerolimov commented Oct 28, 2020

Btw, if the tekton Authentication at Run Time doc contains all cases we also should (in this ticket, or in a new one..) hide some combinations here:

image
(screenshot of the doc toc)

  • When "Access to" "Git Server" is selected we should hide the "Auth Type" option "Image Registry Credentials"
  • When "Access to" "Docker Server" is selected we should hide the "Auth Type" option "SSH auth"

/cc @andrewballantyne @vdemeester wdyt? Should I create a new ticket for this or solve this here as well?

@vdemeester
Copy link
Member

  • When "Access to" "Git Server" is selected we should hide the "Auth Type" option "Image Registry Credentials"
  • When "Access to" "Docker Server" is selected we should hide the "Auth Type" option "SSH auth"

/cc @andrewballantyne @vdemeester wdyt? Should I create a new ticket for this or solve this here as well?

Right, I think it make sense to hide thoses

@jerolimov
Copy link
Member Author

@christianvogt @vdemeester @karthikjeeyar @andrewballantyne Updated the PR with your feedback,

  • it nows also reorders the fields so that you need to select the "Access to" and "Auth type" fields before entering the "Server URL".
  • it hides the "Auth type" "Image Registry Credentials" for Git servers and "SSH auth" for Docker registry.

Btw, should we rename Docker registry to Image registry?!?

@jerolimov
Copy link
Member Author

jerolimov commented Nov 10, 2020

@bgliwa01 Thanks for bringing this up in the weekly pipeline sync. Added the following changes now:

  • Changed section title from "Create Source Secret" to "Create Secret"
  • Restored "Designate provider to be authenticated." as hint for "Access to:"
  • Renamed "Docker Registry" to "Image Registry"

updated-pr

@jerolimov
Copy link
Member Author

/retest

@karthikjeeyar
Copy link
Contributor

Thanks @jerolimov
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
@jerolimov
Copy link
Member Author

/retest

@jerolimov
Copy link
Member Author

/retest

@jerolimov
Copy link
Member Author

Thanks @vikram-raj, I added the missing translation namespace.

Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verified it. Thanks @jerolimov

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2020
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to hit a review on Friday. @jerolimov You still have the same issue that I noted in my description to Brigid. If you switch the type or access-to it does not reset the value of the Server URL... which could be easily overlooked that the hint changed.

@jerolimov
Copy link
Member Author

Forgot to hit a review on Friday. @jerolimov You still have the same issue that I noted in my description to Brigid. If you switch the type or access-to it does not reset the value of the Server URL... which could be easily overlooked that the hint changed.

So you want that we clear the server URL or change them as well? Yeah its maybe the safest way.

We could also apply some additional form validation rules here. But let us start with the easiest solution (resetting it)?! Will do that.

@andrewballantyne
Copy link
Contributor

Forgot to hit a review on Friday. @jerolimov You still have the same issue that I noted in my description to Brigid. If you switch the type or access-to it does not reset the value of the Server URL... which could be easily overlooked that the hint changed.

So you want that we clear the server URL or change them as well? Yeah its maybe the safest way.

We could also apply some additional form validation rules here. But let us start with the easiest solution (resetting it)?! Will do that.

Right, the easiest thing is for us to clear (reset back to default) items below the changed field... that way the user is mentally required to re-address those values. I think you'll probably also want to reset the field back to untouched(?) because Formik won't understand what we are doing and will flag the field against the validation schema -- which says "string not empty"... and errors it because we just emptied it haha. I don't think this noise is needed for the user.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2020
@jerolimov
Copy link
Member Author

@andrewballantyne @vikram-raj @bgliwa01 I added the following change so that the "Server URL" was now cleared when

  • the user switches the "Access to" and this automatically switch the the "Authentication Type"
  • the user manually switches "Authentication Type"

image

@jerolimov
Copy link
Member Author

/retest

@jerolimov
Copy link
Member Author

JavaScript heap out of memory

/retest

@jerolimov
Copy link
Member Author

/test backend

@karthikjeeyar
Copy link
Contributor

Thanks @jerolimov, Verified locally, works fine
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, bgliwa01, jerolimov, karthikjeeyar, vikram-raj

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2020
@openshift-merge-robot openshift-merge-robot merged commit 400c56e into openshift:master Dec 9, 2020
@spadgett spadgett added this to the v4.7 milestone Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants