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

Feature/clarify package generation process #24324

Conversation

parsimeikoikai
Copy link
Contributor

@parsimeikoikai parsimeikoikai commented Dec 3, 2024

Checklist for submitter

Updated the help text to clarify that the command shown is for generating install packages, not enrolling devices. Added a note for Windows users to specify that only MSI packages can be generated.

See #25305

@parsimeikoikai
Copy link
Contributor Author

Hi @jacobshandling ,
I have this PR here. Could you please advise me on what needs to be done regarding this failing step? I have attached a screenshot
Screenshot 2024-12-03 at 21 51 04

if (packageType === "deb") {
return " For CentOS, Red Hat, and Fedora Linux, use --type=rpm.";
} else if (packageType === "msi") {
return " Windows can only generate an MSI package.";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that this conveys information correctly, since we're talking about building an MSI package, which any OS can do (with the correct dependencies), not building from a specific OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iansltx I had attached a wrong issue,the correct issue number is #22022

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm specifically commenting here about the copy for this line; swapping "enroll the host" to "build the installer package" makes sense. I made some updates to the guide linked here recently, hence being picky about how we word things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iansltx ,I think I missed the link would you mind sharing the link again?Also,I have updated the PR

@iansltx iansltx requested a review from noahtalerman December 3, 2024 18:52
@iansltx
Copy link
Member

iansltx commented Dec 3, 2024

@parsimeikoikai A lot of the build steps will fail on forks, including the Docker one, so no need to resolve that one to have the PR in a ready state.

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

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

Project coverage is 63.44%. Comparing base (941713d) to head (13735ce).
Report is 481 commits behind head on main.

Files with missing lines Patch % Lines
.../AddHostsModal/PlatformWrapper/PlatformWrapper.tsx 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #24324    +/-   ##
========================================
  Coverage   63.43%   63.44%            
========================================
  Files        1582     1590     +8     
  Lines      150353   150849   +496     
  Branches     3822     3942   +120     
========================================
+ Hits        95381    95706   +325     
- Misses      47373    47534   +161     
- Partials     7599     7609    +10     
Flag Coverage Δ
frontend 53.22% <87.50%> (+0.52%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -254,7 +254,7 @@ const PlatformWrapper = ({
<>
{packageType !== "plain-osquery" && (
<span className={`${baseClass}__cta`}>
Run this command with the{" "}
Run this command to generate an install package with the updated{" "}
Copy link
Member

Choose a reason for hiding this comment

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

"to generate an install package"

+1 on explaining what running the command will do

Copy link
Member

Choose a reason for hiding this comment

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

I think we should stick with the label/help text copy from the Figma: we settled on keeping the label short but updating the help text to include "Install this package…" to make it clearer what the command does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rachaelshaw ,
According to the figma file linked here, the text should read :

  1. Install this package to add hosts to Fleet. For CentOS, Red Hat, and Fedora Linux, use --type=rpm. (For windows)
  2. Install this package to add hosts to Fleet. For Windows, this generates an MSI package. (For Linux)
  3. Install this package to add hosts to Fleet . (For Macos)

Right?

Copy link
Member

Choose a reason for hiding this comment

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

@rachaelshaw Just a reminder to review this when you're back from the holiday break.

@parsimeikoikai parsimeikoikai force-pushed the feature/clarify-package-generation-process branch from 9428777 to f82c2a4 Compare December 5, 2024 19:35
Copy link
Member

@rachaelshaw rachaelshaw left a comment

Choose a reason for hiding this comment

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

Suggested a few changes to match what's in the designs. Thanks for taking this on!

rachaelshaw
rachaelshaw previously approved these changes Jan 9, 2025
Copy link
Member

@rachaelshaw rachaelshaw left a comment

Choose a reason for hiding this comment

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

Merged in suggested text changes to match the designs, copy lgtm 👍

Copy link
Member

@lukeheath lukeheath 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 for your contribution!

@lukeheath lukeheath merged commit 52fbb23 into fleetdm:main Jan 9, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants