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

Wizard: Add firewall to Review step (HMS-5357) #2769

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

Conversation

regexowl
Copy link
Collaborator

@regexowl regexowl commented Jan 20, 2025

This adds a firewall expandable to the Review step.

JIRA: HMS-5357

@regexowl
Copy link
Collaborator Author

regexowl commented Jan 20, 2025

Rebased on #2767, so that will need to get merged first.

@regexowl
Copy link
Collaborator Author

/jira-epic HMS-5168

@schutzbot schutzbot changed the title Wizard: Add firewall to Review step Wizard: Add firewall to Review step (HMS-5357) Jan 20, 2025
@regexowl regexowl force-pushed the firewall-on-review-step branch 2 times, most recently from cf33a94 to ee797cb Compare January 23, 2025 08:21
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.14%. Comparing base (73e7a51) to head (7dd94cd).

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2769      +/-   ##
==========================================
+ Coverage   83.09%   83.14%   +0.05%     
==========================================
  Files         192      192              
  Lines       22275    22345      +70     
  Branches     2178     2190      +12     
==========================================
+ Hits        18509    18579      +70     
  Misses       3744     3744              
  Partials       22       22              
Files with missing lines Coverage Δ
...ents/CreateImageWizard/steps/Review/ReviewStep.tsx 95.18% <100.00%> (+0.30%) ⬆️
...teImageWizard/steps/Review/ReviewStepTextLists.tsx 95.27% <100.00%> (+0.32%) ⬆️

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 73e7a51...7dd94cd. Read the comment docs.

@regexowl regexowl force-pushed the firewall-on-review-step branch 4 times, most recently from 72765ba to e8662f8 Compare January 29, 2025 14:09
@regexowl regexowl marked this pull request as ready for review January 29, 2025 14:09
Copy link
Contributor

@avitova avitova left a comment

Choose a reason for hiding this comment

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

Also, as I am thinking about it, it might be nice to be able to just add a list of services, divided by some kind of separator. 🤔 But I assume this is MVP, and we still need to see if people are using this?:)

<TextListItem component={TextListItemVariants.dd}>
<CodeBlock>
<CodeBlockCode>
{firewall.ports ? firewall.ports.join(' ') : 'None'}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this was the intention. 🤔

Suggested change
{firewall.ports ? firewall.ports.join(' ') : 'None'}
{firewall.ports.length ? firewall.ports.join(' ') : 'None'}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed 😅 updated!

@mgold1234 mgold1234 force-pushed the firewall-on-review-step branch from e8662f8 to 4058c49 Compare February 2, 2025 09:04
@regexowl regexowl force-pushed the firewall-on-review-step branch from 4058c49 to 7dd94cd Compare February 3, 2025 10:02
@regexowl
Copy link
Collaborator Author

regexowl commented Feb 3, 2025

Also, as I am thinking about it, it might be nice to be able to just add a list of services, divided by some kind of separator. 🤔 But I assume this is MVP, and we still need to see if people are using this?:)

Right, the steps are currently going through the UX review as well. cockpit-composer originally took just a string, I wanted to incorporate some kind of validation as well. But might try to address that in a follow up as I see how that might be really nice to have 🤔 maybe we can just split the entire input string and then just work with the items the same way we currently do?

@regexowl regexowl requested a review from avitova February 3, 2025 12:58
@regexowl
Copy link
Collaborator Author

regexowl commented Feb 3, 2025

Oh wait, rebase is needed first 😳

This adds a firewall expandable to the Review step.
@regexowl regexowl force-pushed the firewall-on-review-step branch from 7dd94cd to 4231359 Compare February 3, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants