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

Add option publish-gh-pages-branch #93

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

felixfontein
Copy link
Collaborator

I've tried integrating updating GH Pages into the shared workflow (_shared-docs-build-publish-gh-pages.yml), and after some tries it seems to work. This PR adds a new option, publish-gh-pages-branch, which needs to be explicitly enabled like this: ansible-collections/community.hrobot@933f952 (obviously without pointing to the workflow in my branch once this is merged ;) )

Successful run: https://github.com/ansible-collections/community.hrobot/actions/runs/9702624870. That run made https://ansible-collections.github.io/community.hrobot/pr/114/firewall_module.html#synopsis show Test! as the last paragraph of the Description (before it was test).

I'm not sure whether this is the best solution, and I'm also a bit surprised that not having environment: github-pages there works. (I had it in the shared workflow first, that made it fail.)

Copy link
Collaborator

@briantist briantist left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me. I'm wondering why we would default the option to false though since the previous behavior for anyone using this workflow would be to publish. If we default to true then we don't need every collection to update, and I think it's also the more intuitive option, no?
(we could also change the option to skip-publish-gh-pages or similar and default to false for the same effect)

@felixfontein
Copy link
Collaborator Author

The main reason for the default false is that additional permissions are needed. If these aren't present, then the workflow will start failing (which among others mean: CI is always failing in PRs). If this isn't merged, then the workflow will effectively stop working since the gh-pages branch is no longer taken as the source for GH Pages, but the workflow won't fail.

Also: it could (theoretically) be that some user of this workflow already implemented the gh-pages branch to GH Pages update in another way - for them this would be a breaking change.

Copy link
Collaborator

@briantist briantist left a comment

Choose a reason for hiding this comment

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

Good points, thanks!

@felixfontein felixfontein merged commit 977cf28 into ansible-community:main Jun 29, 2024
@felixfontein felixfontein deleted the publish-gh-pages branch June 29, 2024 15:02
@felixfontein
Copy link
Collaborator Author

Thanks for reviewing this!

@felixfontein
Copy link
Collaborator Author

Hmm, maybe that argument wasn't right, since without adjustment the docs workflows now also fail: https://github.com/ansible-collections/community.sops/actions/runs/9725081312

The workflow is not valid. .github/workflows/docs-pr.yml (Line: 36, Col: 3): Error calling workflow 'ansible-community/github-docs-build/.github/workflows/_shared-docs-build-publish-gh-pages.yml@main'. The nested job 'publish-gh-pages' is requesting 'pages: write, id-token: write', but is only allowed 'pages: none, id-token: none'.

@gardar
Copy link

gardar commented Jul 1, 2024

Hmm, maybe that argument wasn't right, since without adjustment the docs workflows now also fail: https://github.com/ansible-collections/community.sops/actions/runs/9725081312

The workflow is not valid. .github/workflows/docs-pr.yml (Line: 36, Col: 3): Error calling workflow 'ansible-community/github-docs-build/.github/workflows/_shared-docs-build-publish-gh-pages.yml@main'. The nested job 'publish-gh-pages' is requesting 'pages: write, id-token: write', but is only allowed 'pages: none, id-token: none'.

Yep, also noticed this happening here: https://github.com/prometheus-community/ansible/actions/runs/9740090100
So this is actually a breaking change.

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

Successfully merging this pull request may close these issues.

3 participants