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

Restructure docs #4044

Closed
wants to merge 3 commits into from
Closed

Restructure docs #4044

wants to merge 3 commits into from

Conversation

samccann
Copy link
Contributor

This PR merges existing files with 'empty' future files into one doc set. This is an alternative to the existing docsite which has a large but empty WIP documentation section.

NOTE: this PR still introduces a handful of empty files that readers may find confusing.

@samccann samccann temporarily deployed to ack September 14, 2023 20:46 — with GitHub Actions Inactive
@samccann
Copy link
Contributor Author

This is how the updated left-hand navigation would look:

image

@pre-commit-ci pre-commit-ci bot temporarily deployed to ack September 14, 2023 20:47 Inactive
@samccann
Copy link
Contributor Author

heh that's what i get for doing a PR before running tox -e docs locally...:-P

@ssbarnea ssbarnea temporarily deployed to ack September 15, 2023 09:20 — with GitHub Actions Inactive
@samccann
Copy link
Contributor Author

@ssbarnea - I might need some remedial help on this one. Basically, I moved a batch of files/renamed some etc so I could get a clean left-hand navigation. But this PR has a bunch of broken links problem similar to the other docs scaffolding PR - #3997

So I'm wondering what I need to fix since that other PR I was using to learn from was merged with the similar CI errors to this one?

@samccann samccann mentioned this pull request Oct 25, 2023
Merged
@ciecierski
Copy link
Contributor

@samccann I re-run a tox -e docs on my local env and I think the problem is this line:
https://github.com/ansible/molecule/blob/main/tox.ini#L90
mkdocs build --strict command, which is run first, builds website pages under site directory. All generated website pages in site directory contains absolute links. The problem is that linkchecker tries to travers those links and is getting 404, which is expected as pages are not yet published.
I think we should change L90 to linkchecker -f linkcheckerrc docs. We should only care about links that we put to md files, not those pre-generated by mkdocs command.

@samccann
Copy link
Contributor Author

samccann commented Dec 1, 2023

Thanks @ciecierski ! I tried that fix locally and yes it does remove the errors. So seems we have two ways foward:
1 - Merge (after I fix the conflicts) and ignore the tox errors since we know why they are there.
2 - Change the tox.ini to run only on the docs. This should be done in a separate PR imo if we want to go that route.

I've seen other projects use #1 so they aren't changing the default CI testing. Who do we need to bring into the discussion to decide which way to go?

ciecierski added a commit to ciecierski/molecule that referenced this pull request Dec 4, 2023
@ciecierski
Change linkchecker in tox.ini to run against docs dir
18127a7 
Change linkchecker to run against docs dir to validate only links
provided by user instead of all auto-generated links from site dir.
All auto-generated links in site directory contains absolute links.
The problem is that linkchecker tries to traverse those links and
is getting 404 error, which is expected as pages are not yet published.
This blocks pull-request and adding new pages or restructuring existing
docsite as in this pr[1].

[1]ansible#4044
@ciecierski ciecierski mentioned this pull request Dec 4, 2023
Merged
@ciecierski
Copy link
Contributor

Thanks @ciecierski ! I tried that fix locally and yes it does remove the errors. So seems we have two ways foward: 1 - Merge (after I fix the conflicts) and ignore the tox errors since we know why they are there. 2 - Change the tox.ini to run only on the docs. This should be done in a separate PR imo if we want to go that route.

I've seen other projects use #1 so they aren't changing the default CI testing. Who do we need to bring into the discussion to decide which way to go?

I think we should change tox first to not ignore errors and then rebase this pull-request to fix conflicts and have fixed tox definition. I've already proposed fix to tox in #4093

@ssbarnea Could you help us with making decision/reviews or providing your guidance on this matter?

ciecierski added a commit to ciecierski/molecule that referenced this pull request Dec 4, 2023
@ciecierski
Change linkchecker in tox.ini to run against docs dir
69d00f2 
Change linkchecker to run against docs dir to validate only links
provided by user instead of all auto-generated links from site dir.
All auto-generated links in site directory contains absolute links.
The problem is that linkchecker tries to traverse those links and
is getting 404 error, which is expected as pages are not yet published.
This blocks pull-request and adding new pages or restructuring existing
docsite as in this pr[1].

[1]ansible#4044
ssbarnea pushed a commit that referenced this pull request Dec 4, 2023
@ciecierski
Change linkchecker in tox.ini to run against docs dir (#4093)
631cc73 
Change linkchecker to run against docs dir to validate only links
provided by user instead of all auto-generated links from site dir. All
auto-generated links in site directory contains absolute links. The
problem is that linkchecker tries to traverse those links and is getting
404 error, which is expected as pages are not yet published. This blocks
pull-request for adding new pages or restructuring existing docsite as
in this pr[1].

[1]#4044

Co-authored-by: mciecier <[email protected]>
ssbarnea
ssbarnea requested changes Dec 4, 2023
View reviewed changes
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

Please continue reviewing this. I am adding -1 to it only because I do not want us to accidentally merge addition of empty files.

We should split and merge some of the changes from this as soon we can.

@samccann samccann mentioned this pull request Dec 6, 2023
Open
7 tasks
ciecierski
ciecierski approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@ciecierski ciecierski left a comment

Choose a reason for hiding this comment

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

done

docs/upgrade.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty file. Is there anything worth explicit mentioning for upgrading other then
command pip install --upgrade, such as limitations during upgrade of molecule?

ciecierski
ciecierski approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@ciecierski ciecierski left a comment

Choose a reason for hiding this comment

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

Pull request looks good to me. All empty files removed and it just needs final rebase

@audgirka
Copy link
Contributor

@ssbarnea What needs to be done here?
Or
Can we close this as #4102 is related.

@ssbarnea ssbarnea closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssbarnea ssbarnea ssbarnea requested changes

@cidrblock cidrblock cidrblock approved these changes

@ciecierski ciecierski ciecierski approved these changes

@Qalthos Qalthos Awaiting requested review from Qalthos

@audgirka audgirka Awaiting requested review from audgirka

@shatakshiiii shatakshiiii Awaiting requested review from shatakshiiii

Assignees

@samccann samccann

Labels
needs-update
Projects
🧰 devtools project board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samccann @ciecierski @audgirka @ssbarnea @cidrblock