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.

Sign up for GitHub

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

Jump to bottom

ci: doc to gh actions #1951

Merged
merged 4 commits into from
Jan 12, 2023
Merged

ci: doc to gh actions #1951

merged 4 commits into from
Jan 12, 2023

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Jan 10, 2023 •
edited
Loading 

test: move doc test from travis to actions

Changes:
- drop unnecessary setuptools pin
- use current RTD Python version 3.10.8

Additional Context

Makes doc failures visible sooner via gh actions, plus reduce CI bill. This is mostly just copy / paste from existing gh actions tests and the current travis test - low impact / low effort.

FYI @s-makin I mentioned this would be possible today while we were wrapping up #1933.

@holmanb holmanb requested a review from s-makin January 10, 2023 23:32
TheRealFalcon
TheRealFalcon requested changes Jan 11, 2023
View reviewed changes
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

This might be a little pedantic, but I think we should ensure we're using the same python version that rtd uses to build the docs. In the past, because of dependency issues, we've had issues where local/CI passes but rtd fails on a different python version. That's probably much less likely to happen after removing the old sphinx pins, but I still think it'd make our CI better.

.github/workflows/check_format.yml Show resolved Hide resolved
s-makin
s-makin approved these changes Jan 11, 2023
View reviewed changes
Copy link
Contributor

@s-makin s-makin left a comment

Choose a reason for hiding this comment

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

Definitely a nice idea, avoiding the "install python" step that it does every time. Anything that makes the process faster or more efficient gets a thumbs up from me.

@holmanb
Copy link
Member Author

holmanb commented Jan 11, 2023

This might be a little pedantic, but I think we should ensure we're using the same python version that rtd uses to build the docs. In the past, because of dependency issues, we've had issues where local/CI passes but rtd fails on a different python version. That's probably much less likely to happen after removing the old sphinx pins, but I still think it'd make our CI better.

I will do that, but it may drift since RTD only allows configuring Major.Minor version. I see the most recent build used 3.10.8.

@holmanb holmanb requested a review from TheRealFalcon January 11, 2023 14:09
TheRealFalcon
TheRealFalcon approved these changes Jan 12, 2023
View reviewed changes
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM. With the python version I'm mostly concerned about major.minor.

@holmanb
Copy link
Member Author

holmanb commented Jan 12, 2023 •
edited
Loading

LGTM. With the python version I'm mostly concerned about major.minor

Ah, I guess I was confused about that then, since system Python on Jammy already had the same major.minor as RTD.

@holmanb holmanb merged commit 300b69b into canonical:main Jan 12, 2023
@holmanb holmanb mentioned this pull request Jan 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-makin s-makin s-makin approved these changes

@TheRealFalcon TheRealFalcon TheRealFalcon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@holmanb @TheRealFalcon @s-makin