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

.github/workflows: build NixOS/Nixpkgs manuals #88488

Merged
merged 1 commit into from Oct 2, 2020
Merged

.github/workflows: build NixOS/Nixpkgs manuals #88488

merged 1 commit into from Oct 2, 2020

Conversation

@zowoq
Copy link
Contributor

@zowoq zowoq commented May 21, 2020

Continued from #87853

cc @grahamc @Mic92

Using ofborg is probably better but this might be useful for now?

This action will only start if something in /doc or /nixos/doc is modified so most contributors shouldn't see it.

Each job will skip the cachix action/building the manual unless its paths have changed.

They could be split further into one action per path if we really wanted to save a couple of seconds.

.github/workflows/manual.yml Outdated Show resolved Hide resolved
if: steps.git_diff.outputs.diff
- name: build nixpkgs manual
if: steps.git_diff.outputs.diff
run: nix-shell doc/shell.nix --run 'make -C doc'
Copy link
Member

@Mic92 Mic92 May 21, 2020

According to the manual https://nixos.org/nixpkgs/manual/#chap-contributing we build it with nix-build ./doc. However it also says that make -C doc debug might come in handy in case there are errors.

Copy link
Member

@Mic92 Mic92 May 21, 2020

Copy link
Contributor Author

@zowoq zowoq May 22, 2020

I hadn't noticed that ofborg is running instantiate for the manuals, nixpkgs has pkgs/top-level/release.nix -A manual.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented May 22, 2020

The outcome of the doc format RFC could mean that this isn't needed in a few months which might actually be a point in its favour as it can be easily dropped.

I'm assuming that adding these builds to ofborg is non-trivial and wouldn't be a productive use of time if we might end up changing formats anyway.

@Mic92
Copy link
Member

@Mic92 Mic92 commented May 22, 2020

I also like github workflows for these sort of task more since they don't involve writing Rust and are easier to prototype.

@grahamc
Copy link
Member

@grahamc grahamc commented May 22, 2020

@grahamc
Copy link
Member

@grahamc grahamc commented May 22, 2020

cross-posted from #87853:

I would like to revert this PR and close #88488. Not because I don't like github actions or feel like my pet project ofborg is threatened, but because I think these checks are actually harming the clearness of the checks. I'll explain:

The editorconfig check puts a big green check next to a commit, and hides the list of checks which passed. If ofborg is down or behind, it is likely people will think ofborg has approved the PR before it did. This could cause, well, all the problems ofborg is there to prevent.

The concrete problem here is this green check appears very early and is misleading, and there is no poka yoke to help people do the right thing.

One fear of #88488 is that it will introduce red X's if it takes a long time to build the manual, or something in the target branch is failing to build. This is reminiscent of the Travis times for me: Travis gave out so few green checkmarks, that if Travis gave a PR a green check-mark, it inspired more "okay what is broken that let travis pass it?" than "okay good to merge".

The goal of ofborg, and ever since ofborg became a thing, the "rule" has been green checkmark -> good to merge and red X -> don't merge.

The editorconfig check errodes the clearness of the green checkmark signal.

The big-rebuild failures we'll certainly have with #88488 errodes the clearness of the red X signal.

The clearness of this signal has been my guiding light on the design of ofborg for years now, and I am very afraid of threatening it.

A few suggestions that have been brought up to improve this:

  1. make ofborg faster I'd love to do this, but it doesn't solve this problem if ofborg is seriously delayed or down.
  2. require the ofborg evaluation check for merges this is really the only solution, but not something we can do today, because of some workflow issues we need to resolve.

That makes me think that, today, we should be undoing this check and closing #88488.

@zowoq zowoq closed this May 22, 2020
@zowoq zowoq deleted the manual branch May 22, 2020
@Mic92
Copy link
Member

@Mic92 Mic92 commented Jul 30, 2020

We can now safely run github actions since there is an action that just waits for ofborg. However is this PR here still needed since ofborg already builds the manual?

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Jul 30, 2020

However is this PR here still needed since ofborg already builds the manual?

AFAIK ofborg doesn't?

With the docs format changes ongoing I wasn't planning on doing any work on this for now.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Jul 31, 2020

ok.

@zowoq zowoq restored the manual branch Aug 5, 2020
@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Aug 5, 2020

Hmm, I guess the actual docs format change may not happen for a while but in the meantime we still have broken builds.

May as well give this a go.

Edit: I've just realised that in the last three weeks I've dealt with broken manual builds on three occasions. I've fixed the nixpkgs manual once myself, merged a PR that was also fixing the nixpkgs manual and merged another PR fixing the nixos manual.

@zowoq zowoq reopened this Aug 5, 2020
@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Aug 5, 2020

Now that the install-nix-action is much faster using the full clone / git diff action is the bottleneck. May as well just run the build anyway.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Aug 6, 2020

Binary cache was mentioned in the other builds PR, would running the manual builds on master and staging as an hourly cron job and pushing to the cache address the rebuild concerns?

@Mic92
Copy link
Member

@Mic92 Mic92 commented Aug 6, 2020

Binary cache was mentioned in the other builds PR, would running the manual builds on master and staging as an hourly cron job and pushing to the cache address the rebuild concerns?

This would not be even necessary. We could a cachix signing key that would just upload the manual on every push to master.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Aug 6, 2020

Binary cache was mentioned in the other builds PR, would running the manual builds on master and staging as an hourly cron job and pushing to the cache address the rebuild concerns?

This would not be even necessary. We could a cachix signing key that would just upload the manual on every push to master.

There might be one problem with approach though:

#94658 (comment)

@Mic92
Copy link
Member

@Mic92 Mic92 commented Aug 6, 2020

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Aug 14, 2020

Looks like if we want a job status we need to set it manually or just post a comment when it finishes with pass/fail and a link, would probably need to set another token for it to work.

Edit: also need to set the the pr/commit for the checkout.

@zowoq zowoq changed the title actions: build manuals .github/workflows: build NixOS/Nixpkgs manuals Aug 23, 2020
@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Aug 29, 2020

I've switched this back to running when paths are modified and to use pull_request_target.

@Mic92 Which cachix account would we wan to use, nix-community or something else?

@Mic92
Copy link
Member

@Mic92 Mic92 commented Aug 30, 2020

I've switched this back to running when paths are modified and to use pull_request_target.

@Mic92 Which cachix account would we wan to use, nix-community or something else?

This is up to @domenkozar One cachix key has the advantage that people have to add less caches to add but may be less secure because more people & machines have access to it.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Aug 30, 2020

I think it's best to create a separate account. Not sure who should do that, probably someone from the infrastructure team like @zimbatm

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Sep 23, 2020

I might just create a cache under my name and include the key as plaintext in the workflow for now, we can switch to an official account later if this works.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Sep 23, 2020

@zowoq ok, but you likely need to give the credentials to someone who has administrative rights in nixpkgs like @domenkozar so its accessible from the NixOS org when starting actions from master.

@zowoq zowoq marked this pull request as ready for review Sep 23, 2020
@zimbatm
Copy link
Member

@zimbatm zimbatm commented Oct 1, 2020

@zowoq I added a signing key to the repo. I think it's fine to merge now. External contributors won't be able to push to the cache.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Oct 1, 2020

Thanks @zimbatm.

I've split this into two separate actions so that the entire job is conditional on its path being modified.

@ofborg ofborg bot added the 6.topic: nixos label Oct 2, 2020
Mic92
Mic92 approved these changes Oct 2, 2020
@zimbatm zimbatm merged commit b3d6745 into NixOS:master Oct 2, 2020
16 checks passed
@zowoq zowoq deleted the manual branch Oct 2, 2020
zowoq added a commit that referenced this issue Oct 3, 2020
siraben added a commit to siraben/nixpkgs that referenced this issue Oct 5, 2020
dawidsowa added a commit to dawidsowa/nixpkgs that referenced this issue Oct 11, 2020
dawidsowa added a commit to dawidsowa/nixpkgs that referenced this issue Oct 11, 2020
@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Nov 29, 2020

Apologies for the belated follow up on this, I reverted this shortly after it was merged as it wasn't working correctly and forgot to comment here.

pull_request_target was checking out only the base branch and not the PR branch so it wasn't actually doing anything except verifying that the manuals were building on the base branch. pull_request_target also allows write access to the repo so we can't just force it to check out the PR branch.

pull_request doesn't have write access but it will expose the cache token which is what we were trying to avoid, IIRC there have been some changes on the Cachix side with tokens but I haven't investigated that yet to see if it would help here.

@Mic92
Copy link
Member

@Mic92 Mic92 commented Nov 30, 2020

Apologies for the belated follow up on this, I reverted this shortly after it was merged as it wasn't working correctly and forgot to comment here.

pull_request_target was checking out only the base branch and not the PR branch so it wasn't actually doing anything except verifying that the manuals were building on the base branch. pull_request_target also allows write access to the repo so we can't just force it to check out the PR branch.

Can it not checkout the pull request branch without executing code from it besides building the manual?

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Nov 30, 2020

Maybe I'm misunderstanding what you mean but wouldn't building the manual also build any of it's dependenices that the PR modifies anyway?

@Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 3, 2020

Maybe I'm misunderstanding what you mean but wouldn't building the manual also build any of it's dependenices that the PR modifies anyway?

Yes, but in a nix sandbox, no? The nix binary would be provided by the installer-action. I think the most important security aspect would be that no secrets are in environment variables and than the nix sandbox should take care of the rest.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Dec 3, 2020

I think the most important security aspect would be that no secrets are in environment variables

What about the cachix token?

@Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 3, 2020

I think the most important security aspect would be that no secrets are in environment variables

What about the cachix token?

I think it no longer is exported via environment variables. I think the with statement only sets it for the cachix action itself: https://nix.dev/tutorials/continuous-integration-github-actions.html
Some sort of restricted eval to include files from the systemd should be disabled so.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Dec 6, 2020

Let's try this again: #106036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants