Wizard: Firewall ports input (HMS-5332) #2750
Conversation
|
/jira-epic HMS-5168 |
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #2750 +/- ##
==========================================
+ Coverage 84.86% 84.88% +0.01%
==========================================
Files 184 185 +1
Lines 20827 20886 +59
Branches 2022 2031 +9
==========================================
+ Hits 17675 17729 +54
- Misses 3130 3135 +5
Partials 22 22
Continue to review full report in Codecov by Sentry.
|
90ed59e to
a40af20
Compare
| export const isPortValid = (port: string) => { | ||
| return ( | ||
| port !== undefined && | ||
| /^(\d{1,5}|[a-z]{1,4})(-\d{1,5})?:[a-z]{1,4}$/.test(port) |
There was a problem hiding this comment.
I'm not sure how you figured out the appropriate regex for this but it seems to work! 😅 I tested with these (from our docs):
[customizations.firewall]
ports = ["22:tcp", "80:tcp", "imap:tcp", "53:tcp", "53:udp", "30000-32767:tcp", "30000-32767:udp"]
It would be nice to add a comment with a reference, if you used one.
There was a problem hiding this comment.
I've looked up port ranges, tried to look up list of protocols and put together this. It's not super tight though, I was already thinking about this - do we want to make the regexes very tight (which would usually mean quite long) or reasonably tight and more readable?
Like for example, this is a nicely tight regex for the port range: https://regex101.com/r/laGZzP/1 which we would need to further customize to fit our use case.
There was a problem hiding this comment.
Reasonably tight and more readable! I think our validation goal should be removing footguns (mostly to prevent build failures) while being as un-opinionated as possible otherwise.
| }); | ||
| }); | ||
|
|
||
| describe('Firewall request generated correctly', () => { |
There was a problem hiding this comment.
Would it make sense to also put the test for edit mode in this PR? Import mode, too?
There was a problem hiding this comment.
I wrote edit mode tests when all parts of the step functionality were implemented. That way I wouldn't forget to update them later. Similar with import mode tests, those need a PR to setup on-prem import first.
Would prefer to continue the same workflow here to keep the PRs compact, but I'd say both approaches make sense. So whatever feels better I guess 🤷 😅
There was a problem hiding this comment.
Oh, that's fine then! Just want to make sure we don't forget. 😄
There was a problem hiding this comment.
Got it. I'm following the same checklist we applied to steps that are already in stage-stable 😎
src/Components/CreateImageWizard/steps/Firewall/components/PortsInput.tsx
Outdated
Show resolved
Hide resolved
Accidentally approved, meant to request changes.
a40af20 to
e427198
Compare
|
Thanks for the comments! The smaller nit picks brought up a though - could we / should we tighten up type definitions in the API? We're currently using a mix of imported and custom types in the wizardSlice. The former usually allow undefined as a value, the later mostly don't, which causes inconsistencies in how we handle similar data. 🤔 |
Let's start a discussion about this, maybe we can try to actually use the Github discussions feature! 😅 |
This adds chipping input for ports on the Firewall step.
e427198 to
c1ebb71
Compare
This adds chipping input for ports on the Firewall step.
JIRA: HMS-5332