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/rm firstboot service based on state #2983

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

avitova
Copy link
Contributor

@avitova avitova commented Mar 12, 2025

I noticed there is a bug that does not show a relevant list of services that we create on user's behalf.

How to reproduce:

  1. start creating a blueprint with a first boot script
  2. go to the review step - the custom-first-boot service is not displayed there, but we are adding it on user's behalf
  3. click on "Edit blueprint"
  4. go to First boot step and remove the script
  5. go to the review step - the custom-first-boot service is displayed there, but we will remove it after clicking "edit"

Overall, I thought that the code in requestMapper was a bit unclear, and I needed to do something similar for the Satellite follow-up, so I was digging deeper, and found a small bug.
Let me know what you think. I tried to prevent the code from removing/adding service in case we just edit an existing script.

@avitova avitova force-pushed the move-add-fb-service branch from ff5dae1 to 33372d8 Compare March 12, 2025 11:24
@avitova avitova requested a review from regexowl March 12, 2025 11:24
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.84%. Comparing base (048baff) to head (e581718).
Report is 1 commits behind head on main.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2983      +/-   ##
==========================================
- Coverage   81.85%   81.84%   -0.01%     
==========================================
  Files         210      210              
  Lines       23621    23614       -7     
  Branches     2335     2332       -3     
==========================================
- Hits        19334    19327       -7     
  Misses       4258     4258              
  Partials       29       29              
Files with missing lines Coverage Δ
...onents/CreateImageWizard/steps/FirstBoot/index.tsx 94.52% <100.00%> (+0.49%) ⬆️
...nents/CreateImageWizard/utilities/requestMapper.ts 94.86% <100.00%> (-0.11%) ⬇️

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 048baff...e581718. 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.

@avitova avitova force-pushed the move-add-fb-service branch from 33372d8 to 9b2c146 Compare March 12, 2025 14:41
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.

This works beautifully. Thank you for cleaning up the code ❤️

I noticed there is a bug that does not show a relevant list of services
that we create on user's behalf.

How to reproduce:
1. start creating a blueprint with a first boot script
2. go to the review step - the custom-first-boot service is not displayed there, but we are adding it on user's behalf
3. click on "Edit blueprint"
4. go to First boot step and remove the script
5. go to the review step - the custom-first-boot service is displayed
   there, but we will remove it after clicking "edit"
@regexowl regexowl force-pushed the move-add-fb-service branch from 9b2c146 to e581718 Compare March 13, 2025 08:55
@regexowl regexowl enabled auto-merge (rebase) March 13, 2025 08:56
@regexowl regexowl merged commit 7d6c623 into osbuild:main Mar 13, 2025
21 of 22 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.

2 participants