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

feat: Add disable_nojekyll and cname options #112

Closed
peaceiris opened this issue Feb 19, 2020 · 18 comments
Closed

feat: Add disable_nojekyll and cname options #112

peaceiris opened this issue Feb 19, 2020 · 18 comments
Assignees
Labels
enhancement New feature or request resolved

Comments

@peaceiris
Copy link
Owner

peaceiris commented Feb 19, 2020

Yeah, I had exactly the same behavior in the Settings page.

Another problem I had additionally was that adding a empty commit after fixing the settings didn't seemed sufficient. It triggered the Gihub Actions workflow but not the Github Pages workflow. It seemed necessary to really alter the content of the gh-pages branch in order to publish. It may be useful that say that I'm using Parcel, which generates exactly the same content byte-per-byte given the same source files to optimize caches, and that I use a .nojekyll file to avoid problems.

I think Github has some mechanism to avoid publishing if the content of gh-pages didn't changed, at least when using the .nojekyll file.

By the way, this is completely unrelated but it could be a good idea to add that .nojekyll file by default in your action. 99% of the users will need it, will probably not read the whole documentation so they won't know, and will inevitably encounter a hard to debug problem sooner or later. (I'm convinced all serious users of Github Pages already lost at least one hour of their time because of that damn file like I did ^^ )

Originally posted by @nicolas-van in #9 (comment)

@peaceiris peaceiris self-assigned this Feb 19, 2020
@peaceiris peaceiris added the enhancement New feature or request label Feb 19, 2020
@peaceiris peaceiris changed the title feat: Automatically add .nojekyll and disable option feat: Add .nojekyll and disable option Feb 19, 2020
@dhimmel
Copy link
Contributor

dhimmel commented Feb 19, 2020

I am guessing this will be similar to the --no-jekyll option of ghp-import. The javascript gh-pages utility doesn't have this option, but does have a --dotfiles option that must be specified to copy over files starting with ..

Personally I like the ghp-import design where adding the .nojekyll file is an opt-in option.

@peaceiris
Copy link
Owner Author

peaceiris commented Feb 19, 2020

Yes. Some static site generators like Hugo and Gatsby.js support to adding the .nojekyll file as a static asset. To avoid conflicting with that feature, supporting the add_nojekyll option may be better for us.

In addition, to enhance the option, we can put the explanation about add_nojekyll on the top of the README.

@peaceiris peaceiris changed the title feat: Add .nojekyll and disable option feat: Add add_nojekyll option to add .nojekyll Feb 19, 2020
@dhimmel
Copy link
Contributor

dhimmel commented Feb 19, 2020

You might want to also consider an option to write a CNAME file for configuring custom domains. For this option, the user would want to pass a string that gets written to the CNAME file.

@peaceiris
Copy link
Owner Author

As I said at #25, the same may be said of other files (BingSiteAuth.xml, robots.txt, and so on). I think that it is better to manage those files as files.

What is the difference between (BingSiteAuth.xml, robots.txt) and (CNAME, .nojekyll)? Those all look just files. I do not know whether we should provide the add_nojekyll like feature, or not.

What do you think about this? @dhimmel @nicolas-van

@dhimmel
Copy link
Contributor

dhimmel commented Feb 19, 2020

What is the difference between (BingSiteAuth.xml, robots.txt) and (CNAME, .nojekyll)?

The difference is that CNAME and .nojekyll have special meaning for GitHub Pages. These files are essentially GitHub Pages settings, whereas the other files you mention are actual parts of the static website that would be relevant to other static hosts besides GitHub Pages.

I see your desire to keep things simple and don't have a strong feeling, but do think these options would be widely used if available. This action is designed for deploying to GitHub Pages.

@nicolas-van
Copy link
Contributor

nicolas-van commented Feb 19, 2020

Personally I think that the fact that some static website generators support adding a .nojekyll is kind of a bad design from them (their job should just be to generate HTML). On the other hand it should be the job of a tool designed specifically to upload to Github Pages.

Also I think the Jekyll feature of Github as a whole should be considered completely deprecated since the availability of Github Actions. Why would you use a super specific feature when you can basically do whatever you want ? (Including triggering your own Jekyll build if you like that generator, but using an up to date version with the plugins you choose instead of a fixed set which has always been very limited.)

So my opinion regarding the .nojekyll file generation feature would be:

  • To tolerate if the file already exists (generate it only if it doesn't exist, which means it will never crash)
  • To be an opt-out (only for the 0,1% of users that would like to use Github's custom Jekyll generator for legacy reasons)

Regarding CNAME, now that we speak about it it doesn't seem like a bad idea to add an option to generate it through the custom action. I just think it could be cleaner to have something like that:

- name: Deploy
  uses: peaceiris/actions-gh-pages@v3
  with:
    deploy_key: ${{ secrets.ACTIONS_DEPLOY_KEY }}
    publish_dir: ./public
    cname: example.com

Thank like that:

- run: echo "example.com" > public/CNAME
- name: Deploy
  uses: peaceiris/actions-gh-pages@v3
  with:
    deploy_key: ${{ secrets.ACTIONS_DEPLOY_KEY }}
    publish_dir: ./public

Although I would say it doesn't seem very important, that's just personal taste.

@dhimmel
Copy link
Contributor

dhimmel commented Feb 19, 2020

I think the Jekyll feature of Github as a whole should be considered completely deprecated since the availability of Github Actions.

amen

To be an opt-out (only for the 0,1% of users that would like to use Github's custom Jekyll generator for legacy reasons)

While I agree the majority of users deploying to gh-pages probably want .nojekyll. However, I think this action has a lot of utility as a general purpose deploy-to-branch action. When deploying outputs to branches other than gh-pages, it could be confusing to users to have to add an option to prevent an extra file from being added.

@peaceiris
Copy link
Owner Author

peaceiris commented Feb 19, 2020

Thanks all.

OK. I will add two options: disable_nojekyll and cname. Thanks to the opinions of you all, I got a clear reason to add those. I thought that we should keep this action's role which is just copying the assets until today, but the specific features of GitHub Pages seem to be necessary for us.

Why adding .nojekyll is default behavior?

While I agree the majority of users deploying to gh-pages probably want .nojekyll. However, I think this action has a lot of utility as a general purpose deploy-to-branch action. When deploying outputs to branches other than gh-pages, it could be confusing to users to have to add an option to prevent an extra file from being added.

I agree with this, so I am going to define the behavior like the following:

  • Add .nojekyll file by default for only the master and gh-pages branches. When the file already exists, this action does nothing.
  • When we set other branches to publish_branch, this action does not add .nojekyll file.

How about this spec?

@nicolas-van
Copy link
Contributor

Seems good to me.

@dhimmel
Copy link
Contributor

dhimmel commented Feb 20, 2020

How about this spec?

Smart!

@peaceiris peaceiris changed the title feat: Add add_nojekyll option to add .nojekyll feat: Add disable_nojekyll and cname options Feb 20, 2020
peaceiris added a commit that referenced this issue Feb 20, 2020
- Add .nojekyll file by default for only the master and gh-pages branches. When the file already exists, this action does nothing.
- When we set other branches to publish_dir, this action does not add .nojekyll file.

cf. #112

Co-authored-by: Daniel Himmelstein <[email protected]>
Co-authored-by: Nicolas Vanhoren <[email protected]>
@peaceiris
Copy link
Owner Author

I opened #119 and released v3.3.0-0. Test it!

- name: Deploy
  uses: peaceiris/[email protected]
  with:
    deploy_key: ${{ secrets.ACTIONS_DEPLOY_KEY }}
    # github_token: ${{ secrets.GITHUB_TOKEN }}
    # personal_token: ${{ secrets.PERSONAL_TOKEN }}
    # publish_branch: master
    # publish_dir: ./public
    disable_nojekyll: true
    cname: peaceiris.com

@peaceiris
Copy link
Owner Author

https://github.com/peaceiris/peaceiris.github.io/runs/458272454?check_suite_focus=true#step:6:102

When the CNAME already exists, this action print the waring. Should we stop this action instead of printing the waring?

@dhimmel
Copy link
Contributor

dhimmel commented Feb 21, 2020

I don't think it matters too much, but the warning message could be clearer:

##[warning]CNAME already exists

Could this message be updated to indicate whether CNAME will be overwritten or not?

@peaceiris
Copy link
Owner Author

How about this?

##[warning]CNAME already exists, skip adding CNAME

@peaceiris
Copy link
Owner Author

#119 has been merged, v3.3.0 released, and v3 updated.

Thank you all @nicolas-van and @dhimmel

@peaceiris
Copy link
Owner Author

You are co-author of #119 and added to the contributors.

@peaceiris
Copy link
Owner Author

@github-actions
Copy link
Contributor

This issue has been LOCKED because of it being resolved!

The issue has been fixed and is therefore considered resolved.
If you still encounter this or it has changed, open a new issue instead of responding to solved ones.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request resolved
Projects
None yet
Development

No branches or pull requests

3 participants