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.

Sign up for GitHub

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

Jump to bottom

fix(KFLUXBUGS-1342): add existing secret in the components secret form #1006

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(KFLUXBUGS-1342): add existing secret in the components secret form #1006

wants to merge 1 commit into from
+216 −55

Conversation

JoaoPedroPP
Copy link
Contributor

@JoaoPedroPP JoaoPedroPP commented Oct 14, 2024 •
edited
Loading

Fixes

KFLUXBUGS-1342

Description

When onboarding a new component and adding a build time secret, the dialog cannot find an existing secret in the workspace.

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@JoaoPedroPP JoaoPedroPP marked this pull request as draft October 14, 2024 21:38
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2024
@JoaoPedroPP JoaoPedroPP force-pushed the KFLUXBUGS-1342 branch 3 times, most recently from 673818a to dbbdf42 Compare October 17, 2024 23:57
@JoaoPedroPP
Copy link
Contributor Author

Hello @sahil143

To make the list of existing secrets available for build time I removed the filter on SecretSction.tsx and SecretForm.tsx and add a new variable to block any change of value for existing secrets. Please take a look when you have time and share your thought and concerns if this approach works.

Ps: I didn't rewrite the test yet

@JoaoPedroPP JoaoPedroPP force-pushed the KFLUXBUGS-1342 branch 2 times, most recently from 766ee2e to ffcf5fe Compare October 28, 2024 18:56
@JoaoPedroPP
Copy link
Contributor Author

/retest

@CryptoRodeo
Copy link
Contributor

The current implementation looks good to me so far. I did find this weird bug though when trying to find one of my key/value secrets:

kfluxbugs-1342-select-bug.webm

@JoaoPedroPP
Copy link
Contributor Author

The current implementation looks good to me so far. I did find this weird bug though when trying to find one of my key/value secrets:
kfluxbugs-1342-select-bug.webm

Nice catch :)
I think this was caused because I was not using states for the option values. It should have the correct behavior now

@CryptoRodeo
Copy link
Contributor

CryptoRodeo commented Oct 29, 2024 •
edited
Loading

Yup that fixed it:

kfluxbugs-1342-bug-fixed.webm

@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP JoaoPedroPP force-pushed the KFLUXBUGS-1342 branch 2 times, most recently from 2bb73f0 to 0ea1d9e Compare October 30, 2024 14:46
@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

/retest

@codecov Codecov
Copy link

codecov bot commented Oct 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.00%. Comparing base (8833c6b) to head (af95dff).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/components/Secrets/SecretForm.tsx 87.50% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
- Coverage   85.01%   85.00%   -0.02%     
==========================================
  Files         582      582              
  Lines       14333    14349      +16     
  Branches     4010     4016       +6     
==========================================
+ Hits        12185    12197      +12     
- Misses       2021     2025       +4     
  Partials      127      127
Files with missing lines Coverage Δ
...ponents/ImportForm/SecretSection/SecretSection.tsx 81.08% <100.00%> (+4.15%) ⬆️
src/components/ImportForm/submit-utils.ts 94.59% <100.00%> (+0.30%) ⬆️
src/components/Secrets/SecretModal.tsx 100.00% <100.00%> (ø)
src/components/Secrets/SecretModalLauncher.tsx 80.00% <ø> (ø)
src/components/Secrets/utils/secret-utils.ts 96.92% <100.00%> (+0.07%) ⬆️
src/types/secret.ts 100.00% <ø> (ø)
src/utils/validation-utils.ts 100.00% <100.00%> (ø)
src/components/Secrets/SecretForm.tsx 78.68% <87.50%> (+7.25%) ⬆️

... and 2 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 8833c6b...af95dff. Read the comment docs.

@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP JoaoPedroPP marked this pull request as ready for review November 8, 2024 12:01
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2024
@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

/retest

1 similar comment
@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

@Katka92

Can you help to understand why ci.int.devshift.net test is failing? I did not touch anything related to getting started section.

sahil143
sahil143 suggested changes Nov 11, 2024
View reviewed changes
src/components/Secrets/SecretForm.tsx Outdated Show resolved Hide resolved
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JoaoPedroPP
Once this PR has been reviewed and has the lgtm label, please ask for approval from sahil143. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@JoaoPedroPP
Copy link
Contributor Author

/retest

1 similar comment
@JoaoPedroPP
Copy link
Contributor Author

/retest

@JoaoPedroPP
Copy link
Contributor Author

/retest

@marcin-michal
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
@JoaoPedroPP
fix: list existing secret in build time secret modal
af95dff 
feat: create only non existing build time secrets

feat: alternative solution for list existing secret

test: add missging tests

fix: fix type errors

test: add coverage to missing lines

chore: remove empty array

test: add tests for submit-utils.ts

feat: improve method to select secrets to create
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
@JoaoPedroPP
Copy link
Contributor Author

/retest

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 14, 2024

@JoaoPedroPP: 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-sigs/prow repository. I understand the commands that are listed here.

@CryptoRodeo
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CryptoRodeo CryptoRodeo CryptoRodeo left review comments

@StanislavJochman StanislavJochman StanislavJochman left review comments

@testcara testcara Awaiting requested review from testcara

@Katka92 Katka92 Awaiting requested review from Katka92

@marcin-michal marcin-michal Awaiting requested review from marcin-michal

@sahil143 sahil143 Awaiting requested review from sahil143

Assignees

@sahil143 sahil143

@CryptoRodeo CryptoRodeo

@marcin-michal marcin-michal

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JoaoPedroPP @CryptoRodeo @marcin-michal @sahil143 @StanislavJochman