Skip to content

Wizard: Add AAP registration (HMS-5663)#3226

Merged
kingsleyzissou merged 1 commit intoosbuild:mainfrom
lucasgarfield:aap
Aug 14, 2025
Merged

Wizard: Add AAP registration (HMS-5663)#3226
kingsleyzissou merged 1 commit intoosbuild:mainfrom
lucasgarfield:aap

Conversation

@lucasgarfield
Copy link
Collaborator

@lucasgarfield lucasgarfield commented May 12, 2025

Validation is very rudimentary but present.

Import/export are todo.

Review carefully, some code AI generated.

@lucasgarfield lucasgarfield force-pushed the aap branch 2 times, most recently from 406ff1a to 3c74a61 Compare May 12, 2025 23:08
@lucasgarfield
Copy link
Collaborator Author

lucasgarfield commented May 12, 2025

So far this cost $3.34 in API tokens.

Edit: After more iteration, currently at $6.13.

@lucasgarfield lucasgarfield force-pushed the aap branch 3 times, most recently from ea1e7a8 to 70da31d Compare May 15, 2025 00:04
@lzap
Copy link
Contributor

lzap commented May 15, 2025

Small remark as I am trying this out. It reads "Ansible Controller URL" but then it shows example.towerhost.net string which is not a URL, that is what I call a hostname, so I am confused which one to enter.

image

@lzap
Copy link
Contributor

lzap commented May 15, 2025

This showed up when I clicked next, that is pretty big one, not related I guess :-)

image

@lucasgarfield lucasgarfield force-pushed the aap branch 3 times, most recently from d504707 to 7101907 Compare May 16, 2025 02:21
@lucasgarfield lucasgarfield changed the title WIP: Ansible Automation Platform Wizard: Add AAP registration May 16, 2025
@lucasgarfield
Copy link
Collaborator Author

Small remark as I am trying this out. It reads "Ansible Controller URL" but then it shows example.towerhost.net string which is not a URL, that is what I call a hostname, so I am confused which one to enter.
image

I removed the placeholders for now, let's just keep the UX as basic as possible until we've verified everything is working, then we can polish it up a bit.

@codecov
Copy link

codecov bot commented May 16, 2025

Codecov Report

❌ Patch coverage is 52.11640% with 181 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.82%. Comparing base (11e3524) to head (576fd0d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Components/CreateImageWizard/validators.ts 6.66% 56 Missing ⚠️
...ageWizard/steps/AAP/components/AAPRegistration.tsx 67.62% 44 Missing and 1 partial ⚠️
...teImageWizard/steps/Review/ReviewStepTextLists.tsx 3.12% 31 Missing ⚠️
...ents/CreateImageWizard/utilities/useValidation.tsx 32.60% 31 Missing ⚠️
...ents/CreateImageWizard/steps/Review/ReviewStep.tsx 23.52% 13 Missing ⚠️
...nents/CreateImageWizard/utilities/requestMapper.ts 90.19% 5 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3226      +/-   ##
==========================================
- Coverage   84.32%   83.82%   -0.51%     
==========================================
  Files         198      200       +2     
  Lines       24018    24386     +368     
  Branches     2498     2535      +37     
==========================================
+ Hits        20254    20442     +188     
- Misses       3744     3923     +179     
- Partials       20       21       +1     
Files with missing lines Coverage Δ
...Components/CreateImageWizard/CreateImageWizard.tsx 96.53% <100.00%> (+0.11%) ⬆️
...rc/Components/CreateImageWizard/ValidatedInput.tsx 97.36% <100.00%> (+0.04%) ⬆️
...c/Components/CreateImageWizard/steps/AAP/index.tsx 100.00% <100.00%> (ø)
...nents/CreateImageWizard/utilities/requestMapper.ts 90.65% <90.19%> (+<0.01%) ⬆️
...ents/CreateImageWizard/steps/Review/ReviewStep.tsx 90.46% <23.52%> (-2.76%) ⬇️
...teImageWizard/steps/Review/ReviewStepTextLists.tsx 90.88% <3.12%> (-3.71%) ⬇️
...ents/CreateImageWizard/utilities/useValidation.tsx 89.23% <32.60%> (-4.19%) ⬇️
...ageWizard/steps/AAP/components/AAPRegistration.tsx 67.62% <67.62%> (ø)
src/Components/CreateImageWizard/validators.ts 64.67% <6.66%> (-32.53%) ⬇️

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 11e3524...576fd0d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

'image-builder.satellite.enabled'
);

const isAAPRegistrationEnabled = useFlag('image-builder.aap.enabled') || true; // Default to true for now, remove when flag is implemented
Copy link
Collaborator

Choose a reason for hiding this comment

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

Making a note here for myself so we don't forget to remove the override.

Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

Most of the code looks fine, I've added some comments. The generated request seems to be correct, couldn't test further as I'm not familiar with AAP at all 😳

I presume the on-prem tests are failing because there is no Registration step in on-prem currently.

Also validation doesn't appear when a required field is empty, but I think that's connected to how we handle the pristine state for validated inputs and isn't necessary in the scope of this PR. Since we indicate that the fields are required I'd say it makes sense that the Next button is disabled even without an error message.

@kingsleyzissou
Copy link
Collaborator

Small remark as I am trying this out. It reads "Ansible Controller URL" but then it shows example.towerhost.net string which is not a URL, that is what I call a hostname, so I am confused which one to enter.

This was my fault. Fix here: osbuild/image-builder-crc#1606

@kingsleyzissou
Copy link
Collaborator

I presume the on-prem tests are failing because there is no Registration step in on-prem currently.

Yeah, we should probably fix the tests to account for this

@ochosi ochosi changed the title Wizard: Add AAP registration Wizard: Add AAP registration (HMS-5663) May 22, 2025
@github-actions
Copy link
Contributor

This PR is stale because it had no activity for the past 30 days. Remove the "Stale" label or add a comment, otherwise this PR will be closed in 7 days.

Copy link
Collaborator

@regexowl regexowl left a comment

Choose a reason for hiding this comment

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

Thank you, this is looking really good! Added a few small comments and nitpicks, but overall works nicely ✨

@ksiekl ksiekl force-pushed the aap branch 2 times, most recently from 9f91c76 to d83e39f Compare August 11, 2025 14:45
@ksiekl ksiekl force-pushed the aap branch 2 times, most recently from 68c4fa1 to 41d3b04 Compare August 12, 2025 11:38
@ksiekl ksiekl requested a review from regexowl August 12, 2025 11:54
@ksiekl ksiekl force-pushed the aap branch 2 times, most recently from 0f83dcd to 3f00c08 Compare August 12, 2025 14:22
Copy link
Collaborator

@kingsleyzissou kingsleyzissou left a comment

Choose a reason for hiding this comment

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

This is looking great! Thanks for adding some playwright tests too. I just have some minor comments inline.

kingsleyzissou
kingsleyzissou previously approved these changes Aug 13, 2025
Copy link
Collaborator

@kingsleyzissou kingsleyzissou left a comment

Choose a reason for hiding this comment

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

Amazing 🎉 🚀

@kingsleyzissou kingsleyzissou added this pull request to the merge queue Aug 14, 2025
Merged via the queue into osbuild:main with commit 04adcc1 Aug 14, 2025
32 checks passed
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.

5 participants