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

Update docs GHA and renamed a doc file #4102

Merged
merged 1 commit into from Oct 16, 2023
Merged

Update docs GHA and renamed a doc file #4102

merged 1 commit into from Oct 16, 2023

Conversation

milosgajdos
Copy link
Member

We are missing GH pages setup step in the docs GHA

I went down the rabbit hole of GH pages and came across this:

Which made me think we may be missing this:

We also rename dockerhub.md to distirbution.md.

Jamstah
Jamstah reviewed Oct 13, 2023
View reviewed changes
Copy link
Collaborator

@Jamstah Jamstah left a comment

Choose a reason for hiding this comment

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

Isn't it called dockerhub.md because its the readme for dockerhub? Is there another reason to rename it?

That file is referenced in the dockerhub-readme github action, so that would need updating too I think.

@milosgajdos
Copy link
Member Author

Isn't it called dockerhub.md because its the readme for dockerhub? Is there another reason to rename it?

Oh you're 100% right 🤦‍♂️ that file actually syncs to DH...ok will rename back..not sure what I was thinking. Thanks ❤️

@milosgajdos
Copy link
Member Author

milosgajdos commented Oct 13, 2023 •
edited

@Jamstah I just noticed that GHA (dockerhub-readme) is in the wrong directory 🙃 It should be inside workflows 🫠

GHA need some work by GitHub. Not sure how to prevent that in the future

@milosgajdos
Copy link
Member Author

@Jamstah I just noticed that GHA (dockerhub-readme) is in the wrong directory 🙃 It should be inside workflows 🫠

GHA need some work by GitHub. Not sure how to prevent that in the future

It also references the wrong path

distribution/.github/dockerhub-readme.yml

Line 12 in ebba01e

- '.github/dockerhub-readme.yml'

I am not sure this workflow gets picked up if it's not inside the workflows directory 🤔

@milosgajdos
Copy link
Member Author

Addressed the dockerhub readme kerfuffle in #4103

@Jamstah
Copy link
Collaborator

Jamstah commented Oct 13, 2023

Lets see if it works :)

@milosgajdos
Copy link
Member Author

The debugging story on GH Actions and Pages is not great 😅

@dvdksn
Copy link
Contributor

dvdksn commented Oct 13, 2023

💡 We might need to change the hugo build command. I just realized this github pages is being served from a subpath, but the baseurl of the site is / meaning it expect to be served from root.

Let me raise another PR to let baseurl be controlled by a build arg.

@milosgajdos
Copy link
Member Author

@dvdksn feel free to update this PR

@milosgajdos milosgajdos mentioned this pull request Oct 13, 2023
Closed
@Jamstah
Copy link
Collaborator

Jamstah commented Oct 13, 2023

Dragged in David's changes and added a trigger for the workflow file

Trying to test in my personal org but getting odd deploy errors :/

@milosgajdos
Copy link
Member Author

Yeah these things are sometimes a bit of witchcraft 🙃

@Jamstah
Copy link
Collaborator

Jamstah commented Oct 14, 2023

Tested this on my main branch, looks good: http://jammy.id/distribution/

@milosgajdos
Copy link
Member Author

PTAL @thaJeztah @davidspek

crazy-max
crazy-max reviewed Oct 14, 2023
View reviewed changes
docker-bake.hcl Outdated Show resolved Hide resolved
thaJeztah
thaJeztah reviewed Oct 14, 2023
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

changes look good, but could use a squash of all the commits

@milosgajdos
Copy link
Member Author

I will squash if someone doesn't get to it before I get to my computer, but we need to merge this PR first #4105, otherwise, the CI will keep on failing 😬

@milosgajdos
Copy link
Member Author

milosgajdos commented Oct 15, 2023 •
edited

Hmm ok, my local tree is totally borked . @Jamstah do you have a local copy of the changes and mind rebasing? I think with all these pushes to my fork I somehow totally messed up my branch 🤦‍♂️

Feel free to open a new PR if that's easier. I'm happy to close the mess I somehow created here. Sorry about this, I'm still not sure how this happened 🤔

@Jamstah
Copy link
Collaborator

Jamstah commented Oct 15, 2023

Hmm ok, my local tree is totally borked . @Jamstah do you have a local copy of the changes and mind rebasing? I think with all these pushes to my fork I somehow totally messed up my branch 🤦‍♂️

Feel free to open a new PR if that's easier. I'm happy to close the mess I somehow created here. Sorry about this, I'm still not sure how this happened 🤔

No worries, did you make any further changes?

@milosgajdos
Copy link
Member Author

No worries, did you make any further changes?

I did not. I only tried rebasing and squashing. Still not sure what I screwed up 🤦‍♂️

@milosgajdos @Jamstah
Update docs GHA
777ad03 
Add missing steps to the job, pick up the path automatically, trigger
the job on config file changes.

Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
@Jamstah
Copy link
Collaborator

Jamstah commented Oct 15, 2023

OK, reset back to my local branch, then squashed and rebased.

@milosgajdos
Copy link
Member Author

OK, reset back to my local branch, then squashed and rebased.

Hero! Thank you 🙇‍♂️

PTAL @thaJeztah, but please let's merge #4105 first 😅

davidspek
davidspek approved these changes Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@davidspek davidspek left a comment

Choose a reason for hiding this comment

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

Lgtm

@milosgajdos milosgajdos merged commit 71217a0 into distribution:main Oct 16, 2023
12 checks passed
@milosgajdos milosgajdos deleted the update-docs-vol2 branch October 16, 2023 12:59
@crazy-max
Copy link
Contributor

crazy-max commented Oct 16, 2023 •
edited

@dvdksn Fyi looking at https://github.com/distribution/distribution/actions/runs/6533939030/job/17740105519#step:4:10, we should "touch" a .nojekyll file in our workflow to skip this step as we are using Hugo.

@dvdksn
Copy link
Contributor

dvdksn commented Oct 16, 2023

@crazy-max I wonder why that workflow runs at all. We have this workflow which includes build and deploy: https://github.com/distribution/distribution/actions/workflows/docs.yml

@dvdksn
Copy link
Contributor

dvdksn commented Oct 16, 2023

I think there might be a repository setting to disable this one: https://github.com/distribution/distribution/actions/workflows/pages/pages-build-deployment

@crazy-max
Copy link
Contributor

@crazy-max I wonder why that workflow runs at all. We have this workflow which includes build and deploy: https://github.com/distribution/distribution/actions/workflows/docs.yml

This is expected when deploying to GitHub Pages

@milosgajdos
Copy link
Member Author

Debugging GHA is incredibly painful. We've spent hours on this and both the DH readme and GH pages workflows are still broken 🫠

@milosgajdos
Copy link
Member Author

OK this is sorted now. On @Jamstah 's advice, I've turned on GH actions deployment (instead of branch) and we're in business.

@Jamstah Jamstah mentioned this pull request Oct 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jamstah Jamstah Jamstah approved these changes

@thaJeztah thaJeztah thaJeztah left review comments

@dvdksn dvdksn dvdksn left review comments

@crazy-max crazy-max crazy-max approved these changes

@davidspek davidspek davidspek 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.

None yet
6 participants
@milosgajdos @Jamstah @dvdksn @crazy-max @thaJeztah @davidspek