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 check for building docsite #14406

Merged
merged 13 commits into from
Sep 5, 2023
Merged

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented Sep 1, 2023

SUMMARY

To make sure people don't break the build of the docsite

ping @oraNod

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • Docs

@AlanCoding AlanCoding marked this pull request as ready for review September 1, 2023 20:36
Copy link
Member

@tvo318 tvo318 left a comment

Choose a reason for hiding this comment

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

I like it!

jobs:
docsite-build:
name: docsite test build
runs-on: ubuntu-latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using Fedora? One of my goals is to start using Fedora in all our tests. It'd be cool to start here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

nuts. I could swear I noticed Fedora in another repo somewhere and it's kind of been on my to-do list to start looking at the switch. sorry for the noise.

- name: Assure docs can be built
run: tox -e docs

- name: Upload html build as artifact
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need this step? We can enable PR previews on Read The Docs when we set up hosting. Admittedly I've put this same step in other places but I'm thinking about disabling it.

If we do want to keep it, how about reducing the retention period so we don't have loads of doc builds hanging around and taking up space. The default is 90 days but we could drop that to ~7. https://github.com/actions/upload-artifact#retention-period

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I really wanted your feedback on this. Yes, I'm totally fine to get rid of this and even if we did keep it I agree with dropping the retention time.

I want to defer to whatever method will be used to publish them. So if the readthedocs integration is set up on their side, then it makes no sense to have this. I had in my head that maybe we would have this step, and elsewhere another step that uploaded it to the readthedocs API. That sounds unlikely so I'll push a commit to remove this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AlanCoding Awesome. I can send a PR with the RTD config today and set up the server-side integration with you and TVo as maintainers. Then we can add it to the ansible namespace. Sound OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand all the proper nouns you reference there, but in general it sounds good. I assume that publishing these is next in your agenda and we can sync up in some other communication channels.

Copy link
Collaborator

Choose a reason for hiding this comment

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

apologies, I have a habit of using "RTD" instead of "Read The Docs". I can ping you in Matrix to sync up on the publishing tomorrow if that works.

@AlanCoding AlanCoding merged commit 20fc7c7 into ansible:devel Sep 5, 2023
15 checks passed
djyasin pushed a commit to djyasin/awx that referenced this pull request Sep 16, 2024
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