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 Hostname functionality (HMS-5175) #2672

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

regexowl
Copy link
Collaborator

@regexowl regexowl commented Dec 11, 2024

This adds a validated hostname input and new tests.

JIRA: HMS-5175

@regexowl
Copy link
Collaborator Author

/jira-epic HMS-5164

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 98.48485% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.20%. Comparing base (c98b7d9) to head (1e7c164).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Components/CreateImageWizard/validators.ts 91.66% 1 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2672      +/-   ##
==========================================
+ Coverage   85.13%   85.20%   +0.07%     
==========================================
  Files         175      175              
  Lines       20157    20220      +63     
  Branches     1955     1970      +15     
==========================================
+ Hits        17161    17229      +68     
+ Misses       2974     2969       -5     
  Partials       22       22              
Files with missing lines Coverage Δ
...Components/CreateImageWizard/CreateImageWizard.tsx 99.45% <100.00%> (+<0.01%) ⬆️
...Wizard/steps/Hostname/components/HostnameInput.tsx 100.00% <100.00%> (ø)
...nents/CreateImageWizard/utilities/requestMapper.ts 95.02% <100.00%> (+<0.01%) ⬆️
...ents/CreateImageWizard/utilities/useValidation.tsx 97.41% <100.00%> (+0.29%) ⬆️
src/store/wizardSlice.ts 90.12% <100.00%> (+0.12%) ⬆️
src/Components/CreateImageWizard/validators.ts 92.85% <91.66%> (-0.20%) ⬇️

... and 1 file 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 c98b7d9...1e7c164. Read the comment docs.

@regexowl
Copy link
Collaborator Author

What am I doing wrong with the epic? 🤔

@regexowl
Copy link
Collaborator Author

/jira-epic HMS-5164

@schutzbot schutzbot changed the title Wizard: Add Hostname functionality Wizard: Add Hostname functionality (HMS-5175) Dec 11, 2024
@regexowl
Copy link
Collaborator Author

/retest

This adds a validated hostname input and new tests.
if (!isHostnameValid(hostname)) {
return {
errors: {
hostname: 'Invalid hostname',
Copy link
Collaborator

@lucasgarfield lucasgarfield Dec 11, 2024

Choose a reason for hiding this comment

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

Let's provide some additional guidance to users about what constitutes a valid hostname.

Had to look this up myself, of course. Run man 5 hostname and you get this:

HOSTNAME(5)                                                                         hostname                                                                         HOSTNAME(5)

NAME
       hostname - Local hostname configuration file

SYNOPSIS
       /etc/hostname

DESCRIPTION
       The /etc/hostname file configures the name of the local system. Unless overridden as described in the next section, systemd(1) will set this hostname during boot using
       the sethostname(2) system call.

       The file should contain a single newline-terminated hostname string. Comments (lines starting with a "#") are ignored. The hostname should be composed of up to 64 7-bit
       ASCII lower-case alphanumeric characters or hyphens forming a valid DNS domain name. It is recommended that this name contains only a single label, i.e. without any
       dots. Invalid characters will be filtered out in an attempt to make the name valid, but obviously it is recommended to use a valid name and not rely on this filtering.

       You may use hostnamectl(1) to change the value of this file during runtime from the command line. Use systemd-firstboot(1) to initialize it on mounted (but not booted)
       system images.

So I would suggest the error say: "Invalid hostname. The hostname should be composed of up to 64 7-bit
ASCII lower-case alphanumeric characters or hyphens forming a valid DNS domain name. It is recommended that this name contains only a single label, i.e. without any
dots."


export const isHostnameValid = (hostname: string) => {
if (hostname !== '') {
return /^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])$/.test(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this expression fits what I'm proposing in my other comment. We just need to also set the max length to 64.

@lucasgarfield
Copy link
Collaborator

I added a commit with my suggested changes, hope you don't mind. Just want to keep the review cycle to a minimum so we can ship these before end of Q4.

Hostname validation rules can be found in the hostname man pages (`man 5
hostname`). This commit tweaks the hostname validator function so it is
in line with these guidelines by limiting the length to 64 characters
and also updates the error message for invalid hostnames to provide
users with some additional guidance/context when an invalid hostname is
provided.
Copy link
Collaborator

@lucasgarfield lucasgarfield left a comment

Choose a reason for hiding this comment

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

🫖

@lucasgarfield lucasgarfield enabled auto-merge (rebase) December 11, 2024 18:50
@lucasgarfield
Copy link
Collaborator

Unit tests failing in pr_check but passing in dev-check. Admin merging.

18:51:03 FAIL src/test/Components/CreateImageWizard/steps/ImageOutput/ImageOutput.test.tsx > Step Image output > revisit step button on Review works****

@lucasgarfield lucasgarfield merged commit b4932d6 into osbuild:main Dec 11, 2024
6 of 7 checks passed
@regexowl regexowl deleted the hostname-input branch December 12, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants