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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 9, 2025
Merged

Feature/clarify package generation process #24324

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66aad1c
clarify package installation process
parsimeikoikai Dec 3, 2024
07de9cb
Clarify install package command and add note for MSI packages on Windows
parsimeikoikai Dec 3, 2024
f82c2a4
change the install command instructions
parsimeikoikai Dec 5, 2024
47bccf9
update Clarify Package generation process
parsimeikoikai Dec 6, 2024
d4c5b70
Apply suggestions from code review
rachaelshaw Jan 9, 2025
13735ce
Update frontend/components/AddHostsModal/PlatformWrapper/PlatformWrap…
jacobshandling Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions frontend/components/AddHostsModal/PlatformWrapper/PlatformWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@
interface IPlatformWrapperProps {
enrollSecret: string;
onCancel: () => void;
certificate: any;

Check warning on line 59 in frontend/components/AddHostsModal/PlatformWrapper/PlatformWrapper.tsx

View workflow job for this annotation

GitHub Actions / lint-js (ubuntu-latest) 

Unexpected any. Specify a different type
isFetchingCertificate: boolean;
fetchCertificateError: any;

Check warning on line 61 in frontend/components/AddHostsModal/PlatformWrapper/PlatformWrapper.tsx

View workflow job for this annotation

GitHub Actions / lint-js (ubuntu-latest) 

Unexpected any. Specify a different type
config: IConfig | null;
}

Expand Down Expand Up @@ -254,7 +254,7 @@
<>
{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.

<a
className={`${baseClass}__command-line-tool`}
href="https://fleetdm.com/learn-more-about/installing-fleetctl"
Expand Down Expand Up @@ -338,6 +338,14 @@
}
}`,
};
const getHelpTextForPackageType = (): string => {
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

}
return "";
};

if (packageType === "chromeos") {
return (
Expand Down Expand Up @@ -545,11 +553,7 @@
label={renderLabel(packageType, renderInstallerString(packageType))}
type="textarea"
value={renderInstallerString(packageType)}
helpText={`Distribute your package to add hosts to Fleet.${
packageType === "deb"
? " For CentOS, Red Hat, and Fedora Linux, use --type=rpm."
: ""
}`}
helpText={`Distribute your package to add hosts to Fleet.${getHelpTextForPackageType()}`}
/>
</>
);
Expand Down
Loading