Skip to content

overview: render service demos#668

Merged
fricklerhandwerk merged 16 commits into
ngi-nix:mainfrom
erictapen:overview
Apr 25, 2025
Merged

overview: render service demos#668
fricklerhandwerk merged 16 commits into
ngi-nix:mainfrom
erictapen:overview

Conversation

@erictapen
Copy link
Copy Markdown
Contributor

This is a first draft of the user-facing side of #614. As we don't have set on a virtualisation solution yet, it contains a lot of place holders.

My goal is to refine this step by step as things fall into place, so it's not exactly ready for review, but I welcome your comments.

tmp xdikmgi0Qx

Comment thread overview/default.nix Outdated
ln -s ${fonts} $out/fonts
''
+ (concatLines (mapAttrsToList (path: v: writeHtmlCommand path (htmlFile path v)) pages))
+ (concatLines (mapAttrsToList (path: page: writeProjectCommand path page) pages))
Copy link
Copy Markdown
Contributor

@eljamm eljamm Apr 7, 2025

Choose a reason for hiding this comment

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

This will also generate the top-level index.html file in a directory called index.html, which isn't what we want, probably

$ ls -lR result/
lrwxrwxrwx    - root  1 Jan  1970  fonts -> /nix/store/x5vwknlmicp3wq2z7vqimsz8agyfc1qy-fonts
dr-xr-xr-x    - root  1 Jan  1970  index.html
dr-xr-xr-x    - root  1 Jan  1970  project
.r--r--r-- 9.2k root  1 Jan  1970  style.css

result/index.html:
.r--r--r-- 15k root  1 Jan  1970  index.html

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed that.

@imincik imincik moved this from Needs refinement to In review in Nix@NGI Apr 8, 2025
@imincik imincik moved this from In review to In progress in Nix@NGI Apr 8, 2025
@erictapen erictapen force-pushed the overview branch 2 times, most recently from f19f800 to 8a9b4ff Compare April 18, 2025 10:17
@erictapen erictapen changed the title overview: render service demos overview: render service demos without Nix install instructions for now Apr 18, 2025
@erictapen
Copy link
Copy Markdown
Contributor Author

This should probably be merged before the Nix install workflow is ready, as #698 depends on the default.nix files generated in this.

@erictapen erictapen marked this pull request as ready for review April 18, 2025 11:28
@erictapen
Copy link
Copy Markdown
Contributor Author

tmp j5Oejlxvc1

Comment thread overview/default.nix
Copy link
Copy Markdown
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Nice, just a bunch of text nits for now. The fewer additional words the better here; the demo works and that should speak for itself.

Let's get this out the door

Comment thread overview/default.nix Outdated

machine.wait_for_unit("cryptpad.service")
machine.wait_for_unit("nginx.service")
'';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Run curl to make sure it's alive?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, though I have to admit I never ran this before.

Comment on lines 10 to 11
jinja2_template_file = sys.argv[1]
output_file = sys.argv[2]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can imagine this to be somewhat more natural if it read from stdin and wrote to stdout, but that's maybe cleanup for later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, but currently the script needs the dirname of the output file to read code samples.

Comment thread overview/default.nix Outdated
Comment on lines +272 to +277
<p>
Services utilising the NixOS module system generally only run on NixOS.
If you want to see a quick demo of this project, follow these steps to
run a preconfigured and tested KVM image on your system.
</p>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>
Services utilising the NixOS module system generally only run on NixOS.
If you want to see a quick demo of this project, follow these steps to
run a preconfigured and tested KVM image on your system.
</p>

I'd even go as far as saying we don't need to explain anything here. The heading already says what will happen. (We may want to note that you need virtualization enabled though, but more like in a foot note in case it's hyper slow.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

Comment thread overview/default.nix Outdated
Comment thread overview/default.nix Outdated
Comment thread overview/default.nix Outdated
{{ include_code("nix", "default.nix", relative_path=True) }}
</li>
<li>
<strong>Build the VM start script</strong> defined in <code>default.nix</code> and run it:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<strong>Build the VM start script</strong> defined in <code>default.nix</code> and run it:
<strong>Build the virtual machine</strong> defined in <code>default.nix</code> and run it:

(It's the best kind of correct that we're actually building the wrapper script, but 🤫)

Comment thread overview/default.nix Outdated
Building <strong>will</strong> take a while.
</li>
<li>
<strong>Access the service</strong> from a browser on your host:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<strong>Access the service</strong> from a browser on your host:
<strong>Access the service</strong> with a web browser:

Not sure if it's important to say you need to access it from your machine... can we safely assume that all operations are done on the same machine?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My idea was to communicate to people that there is some vm->host forwarding happening, but I guess people wouldn't try to start a webbrowser in the CLI only VM anyways.

Comment thread overview/default.nix Outdated
demoGlue.one = exampleText: ''
# default.nix
{
ngipkgs ? import (fetchTarball "https://github.com/ngi-nix/ngipkgs/tarball/${self.rev or "main"}") { },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm not sure we want this right now. It will look needlessly noisy and provide no benefit. If we want to protect users from rebuilding everything the next day because we added commits, we should have a release branch instead. But that's for another time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The motivation was to have people come back to a demo weeks later and it still works in the exact same way. Probably not so much of a benefit though.

@imincik imincik linked an issue Apr 25, 2025 that may be closed by this pull request
@imincik
Copy link
Copy Markdown
Contributor

imincik commented Apr 25, 2025

@erictapen , are you able to resolve remaining review comments so we can merge this PR ? Thanks.

@erictapen
Copy link
Copy Markdown
Contributor Author

@erictapen , are you able to resolve remaining review comments so we can merge this PR ? Thanks.

Yes, sorry for it taking so long. I should be through all your comments now, @fricklerhandwerk

@fricklerhandwerk fricklerhandwerk changed the title overview: render service demos without Nix install instructions for now overview: render service demos Apr 25, 2025
@fricklerhandwerk fricklerhandwerk merged commit 5088191 into ngi-nix:main Apr 25, 2025
2 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done in Nix@NGI Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Implement demo instructions on the overview site

4 participants