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 Kernel name input (HMS-5204) #2690

Merged
merged 6 commits into from
Jan 21, 2025

Conversation

regexowl
Copy link
Collaborator

@regexowl regexowl commented Dec 17, 2024

This adds a kernel name input.

JIRA: HMS-5204

@regexowl
Copy link
Collaborator Author

/jira-epic HMS-5192

@schutzbot schutzbot changed the title Wizard: Add Kernel name input Wizard: Add Kernel name input (HMS-5204) Dec 17, 2024
@regexowl regexowl marked this pull request as draft December 17, 2024 08:01
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 97.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.77%. Comparing base (563ff04) to head (d814450).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ImageWizard/steps/Kernel/components/KernelName.tsx 97.03% 4 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2690      +/-   ##
==========================================
+ Coverage   84.66%   84.77%   +0.10%     
==========================================
  Files         186      186              
  Lines       21008    21182     +174     
  Branches     2035     2067      +32     
==========================================
+ Hits        17787    17957     +170     
- Misses       3199     3203       +4     
  Partials       22       22              
Files with missing lines Coverage Δ
...Components/CreateImageWizard/CreateImageWizard.tsx 99.49% <100.00%> (+<0.01%) ⬆️
...nents/CreateImageWizard/utilities/requestMapper.ts 94.69% <100.00%> (+0.08%) ⬆️
...ents/CreateImageWizard/utilities/useValidation.tsx 97.63% <100.00%> (+0.21%) ⬆️
src/Components/CreateImageWizard/validators.ts 94.62% <100.00%> (+0.50%) ⬆️
src/store/wizardSlice.ts 88.78% <100.00%> (+0.07%) ⬆️
...ImageWizard/steps/Kernel/components/KernelName.tsx 97.12% <97.03%> (-2.88%) ⬇️

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 563ff04...d814450. Read the comment docs.

@regexowl
Copy link
Collaborator Author

/retest

@regexowl regexowl marked this pull request as ready for review December 17, 2024 08:19
Comment on lines 112 to 102
return (
kernelName.length < 65 &&
/^[a-z0-9]|[a-z0-9][a-z0-9-]*[a-z0-9]$/.test(kernelName)
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not completely sure about the validation pattern. Didn't find much information in blueprint reference or stages 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I've learned is that when a kernel is specified, DNF is actually used to install it. So it would need to follow whatever the rules for package names in DNF are. I'm sure there must be a standard for that we can find, although my Google-fu has failed me. I did find something for Ubuntu:

https://askubuntu.com/questions/1262528/what-are-usable-characters-are-for-package-names#:~:text=Package%20names%20(both%20source%20and,start%20with%20an%20alphanumeric%20character.

I think if we could find something similar for RH we would have our answer.

@regexowl
Copy link
Collaborator Author

Replaced the input with a simple select in new commit. Can squash later if needed, but wanted to see if this makes more sense first.

@regexowl regexowl marked this pull request as ready for review December 19, 2024 15:13
@mgold1234
Copy link
Collaborator

mgold1234 commented Dec 20, 2024

@regexowl it is looking awsome!
one small thing, I am little confusing why I see the Append without any box, maybe we can remove it fom now for that pr?
Screenshot 2024-12-20 at 11 50 26

@regexowl
Copy link
Collaborator Author

/retest

@lucasgarfield
Copy link
Collaborator

debug, rt, and rt-debug all fail with the same error:
image

I wonder if something in the backend is broken?

@ochosi
Copy link
Contributor

ochosi commented Dec 21, 2024

debug, rt, and rt-debug all fail with the same error:
image

I wonder if something in the backend is broken?

What happens if you try the API directly?

@regexowl
Copy link
Collaborator Author

regexowl commented Jan 3, 2025

That's weird, I've tried to build an image with "kernel-debug" and the build seems successful (RHEL 9 .qcow2)

kernel-name

@regexowl
Copy link
Collaborator Author

regexowl commented Jan 3, 2025

Hmm, but rt fails immediately...

@regexowl
Copy link
Collaborator Author

regexowl commented Jan 6, 2025

Tried to create a blueprint via API (below) and rt failed immediately, same as via UI.

{
  "name": "kernel-name",
  "description": "",
  "distribution": "rhel-9",
  "image_requests": [
  {
    "architecture": "x86_64",
    "image_type": "guest-image",
    "upload_request": {
      "options": {},
      "type": "aws.s3"
    }
  }
  ],
  "customizations": {
    "kernel": {
      "name": "kernel-debug"
    }
  }
}

@regexowl
Copy link
Collaborator Author

/retest

@regexowl regexowl force-pushed the kernel-name-input branch 2 times, most recently from b5e7401 to 0ce54ce Compare January 17, 2025 15:30
@regexowl
Copy link
Collaborator Author

@lucasgarfield I've removed the validator for now as it isn't used anywhere.

@regexowl regexowl marked this pull request as draft January 20, 2025 10:12
@regexowl
Copy link
Collaborator Author

Adding custom name option.

@regexowl regexowl force-pushed the kernel-name-input branch 3 times, most recently from e650688 to 4f7f81e Compare January 20, 2025 11:15
@regexowl
Copy link
Collaborator Author

Allows to add a custom kernel name:
image
image

@regexowl
Copy link
Collaborator Author

Wasn't sure if we want the alert or not. There's a validation for the kernel name format, but even if that passes the build might fail with kernel package that's not included.

@regexowl regexowl marked this pull request as ready for review January 20, 2025 11:53
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

lgtm, though I wonder if something like a text input with a suggestion autocomplete exists? so we could suggest the default options?

@regexowl
Copy link
Collaborator Author

lgtm, though I wonder if something like a text input with a suggestion autocomplete exists? so we could suggest the default options?

Thanks for the review! The default options are there when you click on the input
image

The create option gets rendered when there's a text input that doesn't match any of the default options
image

@regexowl regexowl enabled auto-merge (rebase) January 20, 2025 14:58
@regexowl
Copy link
Collaborator Author

/retest

1 similar comment
@regexowl
Copy link
Collaborator Author

/retest

@regexowl regexowl disabled auto-merge January 20, 2025 15:39
This adds a kernel name input.
This replaces the kernel name input with a static drop down populated with valid values.
We currently don't use this validator.
This replaces previously used single dropdown with a typeahead that allow creating a custom option.
This adds a warning when custom kernel name is selected.
This adds validation for the Kernel step.
@regexowl regexowl merged commit bfef289 into osbuild:main Jan 21, 2025
16 checks passed
@regexowl regexowl deleted the kernel-name-input branch January 21, 2025 07:33
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.

5 participants