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

fix: docs deployment action #1869

Merged
merged 2 commits into from
Nov 17, 2023
Merged

Conversation

r3stl355
Copy link
Contributor

Description

Moved the requirement for contents write permission from release action to docs deployment. The same permission was used in the previous release action (sphinx) so I'm not introducing any extra permission requirements.

One thing I don't really like here is that such permission is only needed for the Deploy step but the lowest level it can be defined is job. This should be fine because other steps in the job don't do any git operations.

Alternative would be creating a separate Deploy job but then I'd have to duplicate all the steps from the Build part which would effectively be the same thing as now but with a longer/redundant config.

Deployment logic remains the same - docs are deployed only when the Build Documentation is run manually or via the Release Python action.

Related Issue(s)

#1867

I am unable to fully test this in my environment because the failing version was actually working in my repo - I don't see how things are configured in the main repo.

Signed-off-by: Nikolay Ulmasov <ulmasov@hotmail.com>
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

I think this should work.

@wjones127 wjones127 merged commit 44a3760 into delta-io:main Nov 17, 2023
24 checks passed
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.

2 participants