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

cache manual results only for pushes #120555

Closed
wants to merge 1 commit into from

Conversation

domenkozar
Copy link
Member

I don't see much benefit for pushing to the cache for PRs.

Comment on lines -3 to -4
permissions: read-all

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep these: these jobs have no need for more than read.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright!

Comment on lines -3 to -4
permissions: read-all

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this too.

Comment on lines -21 to -23
with:
# explicitly enable sandbox
extra_nix_config: sandbox = true
Copy link
Member

Choose a reason for hiding this comment

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

Why delete this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the default value, so it provides no benefit. It would also break macOS in case we'd enable it.

Comment on lines +4 to +5
push:
pull_request:
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? It seems weird that an empty body for an on value would fall through to the next, but I'd believe it if you said so :)

Copy link
Member

@Mic92 Mic92 Apr 25, 2021

Choose a reason for hiding this comment

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

Yes. This is correct syntax. They are not evaluated in order but any of them can trigger the job. They are empty because they could take further parameter like branch lists.

Copy link
Contributor

@zowoq zowoq Apr 25, 2021

Choose a reason for hiding this comment

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

Currently this change will start building the manuals on future release branches, would need to be updated to keep the original behaviour of only building master.

push:
   branches:
      - master

Edit: didn't refresh before posting.

@zowoq
Copy link
Contributor

zowoq commented Apr 25, 2021

I don't see much benefit for pushing to the cache for PRs.

I don't understand what this means or what you're trying to achieve here, can you explain in more detail?

@Mic92
Copy link
Member

Mic92 commented Apr 25, 2021

I guess the motivation is that it would upload too much rebuild manual data to cachix?
Would it be more acceptable if we only cache the tools that are needed to build the manual rather than the manual itself?
This would be helpful for staging branches.

on:
pull_request_target:
push:
pull_request:
branches:
- master
Copy link
Member

Choose a reason for hiding this comment

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

Ah this only happens for master anyway...

on:
pull_request_target:
push:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we only want to run this on master branch than.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not also for releases/staging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Release branch doesn't really see that many changes to it's docs/options after branch off. IMO hydra is sufficient for stable, it's unstable where the docs breakage was reasonably common.

PRs targeting staging would likely timeout frequently with mass rebuilds. Any errors missed in the initial PR on staging are caught by the action in the staging-next -> master PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Mass-rebuilds is something you want to avoid in github actions. Especially without caching.

@domenkozar domenkozar closed this Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants