Skip to content

Conversation

kingsleyzissou
Copy link
Collaborator

@kingsleyzissou kingsleyzissou commented Aug 11, 2025

We can re-enable the registration step for cockpit by allowing users to pass in an activation and org id pair so that they can build an image that is automatically registered.

JIRA: HMS-9030

@kingsleyzissou kingsleyzissou marked this pull request as draft August 11, 2025 15:15
@kingsleyzissou
Copy link
Collaborator Author

Still have to add some playwright tests

@kingsleyzissou
Copy link
Collaborator Author

/jira-epic HMS-8987

@schutzbot schutzbot changed the title Wizard: reintroduce registration step for cockpit Wizard: reintroduce registration step for cockpit (HMS-9030) Aug 11, 2025
@kingsleyzissou kingsleyzissou force-pushed the cockpit-registration branch 2 times, most recently from 92b0a9e to 8308d78 Compare August 11, 2025 15:27
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

❌ Patch coverage is 33.76623% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.60%. Comparing base (a5aa15c) to head (c5de942).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ps/Registration/components/ManualActivationKey.tsx 10.34% 78 Missing ⚠️
...ents/CreateImageWizard/utilities/useValidation.tsx 38.46% 16 Missing ⚠️
...teImageWizard/steps/Review/ReviewStepTextLists.tsx 42.85% 4 Missing ⚠️
...gistration/components/ActivationKeyInformation.tsx 50.00% 2 Missing ⚠️
...nts/CreateImageWizard/steps/Registration/index.tsx 94.44% 1 Missing ⚠️
src/constants.ts 66.66% 1 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3519      +/-   ##
==========================================
- Coverage   82.90%   82.60%   -0.31%     
==========================================
  Files         204      205       +1     
  Lines       24833    24959     +126     
  Branches     2562     2573      +11     
==========================================
+ Hits        20589    20617      +28     
- Misses       4223     4321      +98     
  Partials       21       21              
Files with missing lines Coverage Δ
...Components/CreateImageWizard/CreateImageWizard.tsx 97.08% <100.00%> (+0.54%) ⬆️
...s/CreateImageWizard/steps/Review/Footer/Footer.tsx 98.00% <100.00%> (+1.09%) ⬆️
...nents/CreateImageWizard/utilities/requestMapper.ts 90.68% <100.00%> (+0.03%) ⬆️
...nts/CreateImageWizard/steps/Registration/index.tsx 98.30% <94.44%> (-1.70%) ⬇️
src/constants.ts 99.59% <66.66%> (-0.41%) ⬇️
...gistration/components/ActivationKeyInformation.tsx 85.34% <50.00%> (-1.39%) ⬇️
...teImageWizard/steps/Review/ReviewStepTextLists.tsx 90.45% <42.85%> (-0.44%) ⬇️
...ents/CreateImageWizard/utilities/useValidation.tsx 87.19% <38.46%> (-2.05%) ⬇️
...ps/Registration/components/ManualActivationKey.tsx 10.34% <10.34%> (ø)

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 a5aa15c...c5de942. 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.

@kingsleyzissou kingsleyzissou force-pushed the cockpit-registration branch 15 times, most recently from c71c924 to d2bcd05 Compare August 18, 2025 15:38
@kingsleyzissou kingsleyzissou marked this pull request as ready for review August 18, 2025 17:28
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.

Nice! Added some small nit picks :)

As for copy - looks good to me, we can also check with UX or with someone from docs to get their feedback.

@kingsleyzissou
Copy link
Collaborator Author

Nice! Added some small nit picks :)

As for copy - looks good to me, we can also check with UX or with someone from docs to get their feedback.

The activation key copy should be more or less fine I think since I got that from the official docs. I guess it's the org id that I was less sure about. Will ping someone to have a look once I fix the nitpicks

@regexowl
Copy link
Collaborator

Oh right, looking at the unit test failure - the "additional connection options" should be hidden by default. So the original useState logic was correct. 🤔 The options opening on their own is a reasonably small bug so maybe we could revisit it in a follow up. Something is probably messing up with the state setter.

@kingsleyzissou
Copy link
Collaborator Author

Oh right, looking at the unit test failure - the "additional connection options" should be hidden by default. So the original useState logic was correct. 🤔 The options opening on their own is a reasonably small bug so maybe we could revisit it in a follow up. Something is probably messing up with the state setter.

Ya I might try look at it now, graphite decided to throw a few more useful comments at me too

@kingsleyzissou
Copy link
Collaborator Author

Oh right, looking at the unit test failure - the "additional connection options" should be hidden by default. So the original useState logic was correct. 🤔 The options opening on their own is a reasonably small bug so maybe we could revisit it in a follow up. Something is probably messing up with the state setter.

Actually, maybe I can drop that commit and do it in a follow up like you suggested. It's kind of weird

Copy link
Collaborator

@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.

Few nitpicks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume that when one of these values is invalid, we should disable the button.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good shout, thanks will have a look at this today

@kingsleyzissou kingsleyzissou force-pushed the cockpit-registration branch 2 times, most recently from 42a3499 to 272c9cb Compare August 20, 2025 12:58
@kingsleyzissou
Copy link
Collaborator Author

Graphite is loving this PR 🙄

@avitova
Copy link
Collaborator

avitova commented Aug 20, 2025

@kingsleyzissou I feel like it got a bit smarter, it is spamming throughout my PRs lately a bit more as well.

@kingsleyzissou
Copy link
Collaborator Author

@kingsleyzissou I feel like it got a bit smarter, it is spamming throughout my PRs lately a bit more as well.

In fairness it made some good catches!

@avitova
Copy link
Collaborator

avitova commented Aug 20, 2025

Yeah, it did. Okay, my last question is - because I assume we do not want to expose activation keys when the user clicks "Edit blueprint", is there a plan on what to do, so that user is able to both a) keep activation keys after editing a blueprint, but also b) check "Register later"?
But overall I am good here.

@kingsleyzissou
Copy link
Collaborator Author

Yeah, it did. Okay, my last question is - because I assume we do not want to expose activation keys when the user clicks "Edit blueprint", is there a plan on what to do, so that user is able to both a) keep activation keys after editing a blueprint, but also b) check "Register later"?
But overall I am good here.

Ya great questions. For point a) I'm not entirely sure how we want to handle this. Since this should theoretically only be visible to the user that creates the blueprint, we should be fine. But maybe we do want to hide this?

For b) there is a small back when going back and clicking register later. I will fix that in a follow up

@croissanne
Copy link
Member

Honestly I think it's fine. Activation keys are not traditional secrets like passwords are.

@kingsleyzissou kingsleyzissou force-pushed the cockpit-registration branch 3 times, most recently from c35a627 to 0bdd8d3 Compare August 20, 2025 14:03
We can re-enable the registration step for cockpit by allowing users to
pass in an activation and org id pair so that they can build an image
that is automatically registered.
@kingsleyzissou
Copy link
Collaborator Author

So I think this might not land for the ITM release, I have done a run through and we can add the subscription details.
But it looks like the final image doesn't get subscribed. Will need to do some digging

@kingsleyzissou
Copy link
Collaborator Author

I found the issue, we're creating the ComposeRequest with a blueprint rather than customizations field so the subscription options aren't propagated the whole way through. We need to change the behaviour and send a composer request with customizations set instead. This might be a big change and I think it's definitely better to wait until after the ITM release.

@kingsleyzissou kingsleyzissou changed the title Wizard: reintroduce registration step for cockpit (HMS-9030) Wizard: reintroduce registration step for cockpit (HMS-9195) Aug 26, 2025
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.

4 participants