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

Jump to bottom

redocly: 1.29.0 -> 1.34.0 #391700

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

redocly: 1.29.0 -> 1.34.0 #391700

wants to merge 8 commits into from
+22 −16

Conversation

themadbit
Copy link

@themadbit themadbit commented Mar 21, 2025 •
edited
Loading

Resolves issue.
This PR updates the package to the latest release and fixes the issue with the program's naming, as seen when the --help option is used.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Mar 21, 2025
@nix-owners nix-owners bot requested a review from szlend March 21, 2025 00:34
@themadbit
Copy link
Author

themadbit commented Mar 21, 2025 •
edited
Loading

@szlend kindly review. This PR is my step 1 to fixing issue

@themadbit themadbit mentioned this pull request Mar 21, 2025
Open
szlend
szlend reviewed Mar 21, 2025
View reviewed changes
pkgs/by-name/re/redocly/package.nix Outdated
@@ -1,5 +1,6 @@
{
lib,
pkgs,
Copy link
Member

Choose a reason for hiding this comment

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

Use nodejs directly, not through pkgs.

Copy link
Author

Choose a reason for hiding this comment

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

ok. Working on that

szlend
szlend reviewed Mar 21, 2025
View reviewed changes
pkgs/by-name/re/redocly/package.nix Outdated
Comment on lines 48 to 49
# Add telemetry and update notice flags
wrapProgram $out/bin/redocly \
--set-default REDOCLY_TELEMETRY off \
--set-default REDOCLY_SUPPRESS_UPDATE_NOTICE true
'';
Copy link
Member

@szlend szlend Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we already have a custom nodejs wrapper script, then we might as well move this functionality too.

Something like this should work right?

process.env.REDOCLY_TELEMETRY = process.env.REDOCLY_TELEMETRY || "off";
process.env.REDOCLY_SUPPRESS_UPDATE_NOTICE = process.env.REDOCLY_SUPPRESS_UPDATE_NOTICE || "true ";

Then remove the makeWrapper dependency entirely.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for this. I hadn't considered the nodejs wrapper. Let me look at it

Copy link
Author

Choose a reason for hiding this comment

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

check latest commit. Formatting fails but I'm working on it

@themadbit themadbit force-pushed the bump-redocly branch 3 times, most recently from a115a33 to 844cf85 Compare March 21, 2025 16:19
szlend
szlend reviewed Mar 21, 2025
View reviewed changes
pkgs/by-name/re/redocly/package.nix Outdated
Comment on lines 45 to 44
process.env.REDOCLY_TELEMETRY = "off";
process.env.REDOCLY_SUPPRESS_UPDATE_NOTICE = "true"
Copy link
Member

Choose a reason for hiding this comment

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

While I doubt anyone will want to override these, I think it's still good practice to allow users to do so.

szlend
szlend reviewed Mar 21, 2025
View reviewed changes
pkgs/by-name/re/redocly/package.nix Outdated

npmBuildScript = "prepare";

nativeBuildInputs = [ makeWrapper ];
nativeBuildInputs = [ ];
Copy link
Member

Choose a reason for hiding this comment

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

Remove

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@szlend
Copy link
Member

szlend commented Mar 21, 2025

I've tested the package and apart from some nits, the changes look good to me. Thank you for the improvements!

@themadbit themadbit force-pushed the bump-redocly branch 2 times, most recently from 52aa50e to b3413ba Compare March 23, 2025 08:28
@themadbit
Copy link
Author

Thanks for the guidance. You can merge now?

@szlend
Copy link
Member

szlend commented Mar 23, 2025

Thanks for the guidance. You can merge now?

The issue with default env vars is still unresolved. Please change to something like:

process.env.REDOCLY_TELEMETRY = process.env.REDOCLY_TELEMETRY || "off";
process.env.REDOCLY_SUPPRESS_UPDATE_NOTICE = process.env.REDOCLY_SUPPRESS_UPDATE_NOTICE || "true ";

@themadbit
Copy link
Author

I had not seen the comment. Fixed now
I've tested with export REDOCLY_SUPPRESS_UPDATE_NOTICE=false and it works.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Mar 23, 2025
@themadbit
Copy link
Author

@szlend I now wait for a merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szlend szlend

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

redocly: provide completions
4 participants
@themadbit @szlend @wegank @NixOSInfra