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

PR previews have undesired side effect for main page (deleted html pages are persisting) #120

Closed
jbusecke opened this issue Sep 16, 2024 · 19 comments · Fixed by #125
Closed
Assignees
Labels
bug Issues that present a reasonable conviction there is a reproducible bug.

Comments

@jbusecke
Copy link
Contributor

jbusecke commented Sep 16, 2024

I am tracking an issue with our docs over at LEAP here. We just noticed that the deployment branch retains html files which are not currently part of the book anymore!

I figure this is due to the setting here which I think is necessary for the preview to work.

I think the desired solution here would be to specify a directory (/preview) that is retained, while all other files represent a current commit? It seems that this has been brought up as feature request upstream (peaceiris/actions-gh-pages#718 and peaceiris/actions-gh-pages#771) but is not currently implemented.

Wondering what folks here think about this? This seems like a pretty bad behavior to me (and has caused some major confusion with our users), so I would be willing to implement a custom solution until an upstream solution is available. But I think such a solution should definitely live here?

@jbusecke jbusecke added the bug Issues that present a reasonable conviction there is a reproducible bug. label Sep 16, 2024
@jbusecke
Copy link
Contributor Author

I just saw this which might be a possible solution!

@brian-rose
Copy link
Member

Thanks for bringing this up, @jbusecke! I completely agree that this is bug in the current implementation.

The potential switch to https://github.com/JamesIves/github-pages-deploy-action instead of https://github.com/peaceiris/actions-gh-pages that you referred to above looks promising to me. Both actions have similar functionality and both seem to be in wide use, but https://github.com/JamesIves/github-pages-deploy-action looks to be under more active current development.

@erogluorhan I think it makes sense for the Pythia infrastructure team to take a look at this.

@erogluorhan
Copy link
Member

Thanks @jbusecke ! @brian-rose, I also agree that we need to look into this. Hey @ProjectPythia/infrastructure , anyone available to look into this till the next iteration?

@jukent
Copy link
Contributor

jukent commented Oct 10, 2024

@jbusecke to test whether my fix in #125 works, could you temporarily run your workflow with the small change as in https://github.com/jukent/cookbook-template/pull/14/files

I am able to verify that the new action runs successfully, but do not know what files to look for to make sure they are no longer persisting.

@erogluorhan
Copy link
Member

@jbusecke and @jukent just an FYI, I've merged PR #125 that led this one to get closed, but depending on @jbusecke future observations, we can re-open

@jbusecke
Copy link
Contributor Author

Just running the deploy action from main:

  deploy:
    needs: build
    uses: ProjectPythia/cookbook-actions/.github/workflows/deploy-book.yaml@main
    with:
      publish_dir: "book/_build/html"

here (with no changes in our repo) I am getting an error:

fatal: not in a git directory

https://github.com/leap-stc/leap-stc.github.io/actions/runs/11390409843/job/31692035052#step:5:43

I am not quite sure what is happening here yet, but it seems that #125 has broken backwards compatibility somehow?

@jbusecke
Copy link
Contributor Author

The only difference I see with @jukent s setup is that I define a custom publish_dir.

@jukent
Copy link
Contributor

jukent commented Oct 17, 2024

It looks like you might need to use target-folder instead of publish_dir to be compatible with the new backend action (https://github.com/JamesIves/github-pages-deploy-action)

@jbusecke
Copy link
Contributor Author

jbusecke commented Oct 17, 2024

I noticed this warning https://github.com/leap-stc/leap-stc.github.io/actions/runs/11390409843/job/31692035052#step:5:1 which indicates that there a few arguments that are not recognized in the new action. It would be great if we can accomodate the switch to the new deploy action without changing the top level API here.

Trying this over here: #129

One issue is that I am a bit unsure how to accomodate the CNAME input: https://github.com/peaceiris/actions-gh-pages#%EF%B8%8F-add-cname-file-cname

@jbusecke
Copy link
Contributor Author

Have to step away from this for a bit, but will hopefully follow up in a bit as this is currently breaking our build (I can easily revert back to a previous version though).

@erogluorhan erogluorhan reopened this Oct 17, 2024
@github-project-automation github-project-automation bot moved this from Done to In Progress in Pythia Projects Board Oct 17, 2024
@erogluorhan erogluorhan moved this from In Progress to In Review in Pythia Projects Board Oct 17, 2024
@jbusecke
Copy link
Contributor Author

I have an open PR #129 which fixes the issues on our end, but there are some arguments that I do not quite know what to do with. Feedback much appreciated.

@brian-rose
Copy link
Member

@jbusecke I spent time on this today. After some back and forth (#129, #131, #134) I think things are working as intended now.

Please report back here when you get a chance about any outstanding issues.

@jbusecke
Copy link
Contributor Author

@brian-rose thanks a lot. Ill quickly build our docs from main here and will report back. Can we also release a version after that?

@jbusecke
Copy link
Contributor Author

I am getting the following warning in the Run CumulusDS/[email protected] step:

Warning: Unexpected input(s) 'execute_notebooks', 'binderhub_url', 'timeout', valid inputs are ['file']

and also:

The following actions use a deprecated Node.js version and will be forced to run on node20: CumulusDS/[email protected]. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/

@brian-rose should I open a separate issue for this?

but generally our production deployment seems to work.

I also tested our preview here and it seems to be fine (or are there other side effects I should be checking for?

@brian-rose
Copy link
Member

Hi @jbusecke, yes I've seen those warnings too. I found this issue open upstream. The workaround seems to be working (for now).

@brian-rose
Copy link
Member

I also tested our preview here and it seems to be fine (or are there other side effects I should be checking for?

Great! I'm just wondering if we're ready to close this issue. I think the original problem of persistent html pages has been solved, but I haven't 100% verified this myself.

@jbusecke
Copy link
Contributor Author

Oh shoot, I just realized I am still running from my PR branch! https://github.com/leap-stc/leap-stc.github.io/blob/main/.github/workflows/publish-book.yaml

Ill retest just to make sure. After that I think this is ok to close.

@jbusecke
Copy link
Contributor Author

jbusecke commented Oct 28, 2024

Testing now using the most recent main branch.

@jbusecke
Copy link
Contributor Author

Ok the tests were all successful on our end. Closing this now!

@github-project-automation github-project-automation bot moved this from In Review to Done in Pythia Projects Board Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that present a reasonable conviction there is a reproducible bug.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants