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

add build secret section in the import flow #595

Merged

Conversation

karthikjeeyar
Copy link
Contributor

@karthikjeeyar karthikjeeyar commented Apr 28, 2023

Fixes

https://issues.redhat.com/browse/HAC-3481

Description

As a user, I want to add my token to the namespace so that I can use it in my build pipeinerun's partner tasks (eg: snyk).

PR contains following changes

  1. Supports only Opaque secrets at the moment.
  2. List the snyk-secret in the select box, when the user selects the task a predefined token key will be presented to the user and they can enter the token data.
  3. Users can also enter the custom secret name and add one or more key/value pairs.
  4. Drag and drop or Upload a file in value section.

Type of change

  • Feature

Screen shots / Gifs for design review

Handling.build.secrets.mov

Error State:

Screenshot 2023-04-29 at 12 34 54 AM

Snyk secret (predefined secret):
Screenshot 2023-04-29 at 12 34 32 AM

After linking the newly created secret in pipelinerun yaml files, the snyk task is executed:

snyk_task

How to test or reproduce?

  1. In Import flow, Add secret option is available under advanced options.

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

cc: @rohitkrai03 @sbose78 @christianvogt @Misjohns

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2023
@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Merging #595 (7934957) into main (b90c202) will increase coverage by 0.81%.
The diff coverage is 85.07%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #595      +/-   ##
==========================================
+ Coverage   81.12%   81.94%   +0.81%     
==========================================
  Files         468      473       +5     
  Lines       11212    11382     +170     
  Branches     3186     3217      +31     
==========================================
+ Hits         9096     9327     +231     
+ Misses       1992     1933      -59     
+ Partials      124      122       -2     
Impacted Files Coverage Δ
src/components/ImportForm/GitImportForm.tsx 76.71% <ø> (ø)
src/components/ImportForm/utils/types.ts 100.00% <ø> (ø)
src/shared/components/formik-fields/field-types.ts 100.00% <ø> (ø)
...rmik-fields/text-column-field/text-column-types.ts 100.00% <ø> (ø)
...ts/ComponentSettingsForm/ComponentSettingsView.tsx 56.36% <25.00%> (-1.97%) ⬇️
...ared/components/formik-fields/SelectInputField.tsx 81.25% <68.18%> (+61.80%) ⬆️
...ponents/ImportForm/SecretSection/SecretSection.tsx 73.33% <73.33%> (ø)
src/components/Secrets/SecretModal.tsx 80.00% <80.00%> (ø)
src/components/Secrets/OpaqueSecretForm.tsx 88.88% <88.88%> (ø)
...-value-file-input-field/KeyValueFileInputField.tsx 88.88% <88.88%> (ø)
... and 8 more

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b90c202...7934957. Read the comment docs.

@karthikjeeyar karthikjeeyar force-pushed the add-pipeline-secrets branch 2 times, most recently from d23f73e to 961fb8f Compare April 30, 2023 15:55
@Misjohns
Copy link

Misjohns commented May 2, 2023

For the secret section:

  • Secret remove is too far away from the field; use the same placement as the environment variable field placement.
  • Secret fields are very long. Let's align those with the variable name field.

Secret bug

@karthikjeeyar
Copy link
Contributor Author

@Misjohns I have fixed the width of the secret field and remove button placement.

image

@karthikjeeyar karthikjeeyar force-pushed the add-pipeline-secrets branch 3 times, most recently from fc1ccd8 to f786c49 Compare May 3, 2023 10:48
@karthikjeeyar
Copy link
Contributor Author

I rebased this PR with the new Import flow changes #592

@karthikjeeyar
Copy link
Contributor Author

/retest

@karthikjeeyar karthikjeeyar force-pushed the add-pipeline-secrets branch 4 times, most recently from 786090b to a42d66f Compare May 4, 2023 06:19
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karthikjeeyar, rohitkrai03

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:
  • OWNERS [karthikjeeyar,rohitkrai03]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karthikjeeyar
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2023

@karthikjeeyar: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit c82882e into openshift:main May 4, 2023
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants