-
Notifications
You must be signed in to change notification settings - Fork 7
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
Potential follow up fix for #125 #129
Conversation
clean: true | ||
clean-exclude: "preview" # keep existing previews from other PRs | ||
target-folder: ${{ inputs.destination_dir }} | ||
CNAME: ${{ inputs.cname }} # how can we set this for the new action? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CNAME does not have an analog in the new action. Does someone have an example deployment that uses this input to test. There is some more info here: https://github.com/JamesIves/github-pages-deploy-action#additional-build-files-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could git-config-name
be similar to what you are looking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am honestly really unsure. Could somebody test this out on a doc that uses this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our Foundations site uses this feature. Currently fixing some broken links in Foundations that are preventing CI tests from passing (ProjectPythia/pythia-foundations#495), once those are merged I'll do some testing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bit of reading on the CNAME feature, including the docs that @jbusecke linked above. Previously we were passing this argument through to peaceiris/actions-gh-pages. As far as I understand, the only effect of this argument is to ensure that a CNAME file is created at the root of the deployment branch (we use gh-pages
, e.g. this file in the Foundations repo).
That file hasn't changed in three years because we've never changed the custom domain name.
The docs for peaceiris/actions-gh-pages also say
To add the CNAME file, we can set the cname option. Alternatively, put your CNAME file into your publish_dir. (e.g. public/CNAME)
The docs for JamesIves/github-pages-deploy-action also refer to manual commit of the CNAME file into the deployment branch:
If you're using a custom domain and require a CNAME file, or if you require the use of a .nojekyll file, you can safely commit these files directly into the deployment branch without them being overridden after each deployment
My conclusion is that we should just remove this input argument. Manually creating the CNAME file in the gh-pages
branch is a one-time step. And within Project Pythia, we only use this feature for the Foundations and Cookbook Gallery repos. It's not something that we are actively supporting for Cookbook contributors.
I think this will fix the preview deployment problem #130. In the interest of moving things forward, I'm going to merge this now. I don't understand why it should be necessary to checkout the repo during the deploy workflow, but I'll take a look at that separately. I will also work on removing the |
This is an attempt to fix our deployment over at https://github.com/leap-stc/leap-stc.github.io which broke after #125 (I foolishly used @main)
I think that the input names needed to be adjusted, but crucially I had to checkout the repo in the deploy workflow to get this to run.
I have confirmed that our main deployment and a test PR.
Overall I think this emphasizes the need for #6. Some sort of an end-to-end test would have presumably caught this?