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

GithubPagesPublisher refactor #995

Merged
merged 9 commits into from Aug 2, 2022

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Jan 31, 2022

What started out as work to support explicitly specifying a branch to push to (#978) has turned into this.

Issue(s) Resolved

Fixes #978

Related Issues / Links

Associated doc PR: lektor/lektor-website#340

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Link to corresponding documentation pull request for getlektor.com
  • Add support for specifying which branch to push to by specifying a branch query param on the target URL.
  • Issues a warning when defaulting to pushing to the master branch for a user or organization pages site. (Recently created user or organization pages repositories default to using the main branch.)
  • [Optimization] Do a shallow fetch when fetching the current target branch from upstream.
  • [Optimization] Avoid copying the HTML output files to a new scratch directory.
  • Refactor/cleanup of some code in lektor.publisher.

Question

Is it worth adding an option that would prevent keeping any git history of the deployed site?

If one doesn't care about preserving the history of the deployed gh-pages site, one can just force-push the new
content to the target branch. This avoids having to fetch the existing branch from upstream, saving network bandwidth.

In general, I'm not sure how useful it is to keep a history of the deployed site anyway.

Of pertinence, I just noticed that our docs currently say: "The way this is implemented in Lektor currently is that Lektor will force-push a website into a repository of choice."
This appears not to be what our current code does. Current code appears to alway creates a new commit (with the previous branch head as its parent) preserving all previously published version of the site's HTML in the git history. (No force push involved.)
Later on the same page, our docs say: "The way this deployment support works is that it commits a new commit into a temporary location and pushes it into the gh-pages or master branch depending on the name of the repository." (which is not quite the same thing.)

@dairiki dairiki force-pushed the feature.ghp-publisher-refactor branch 6 times, most recently from 10a25fd to 6a953eb Compare February 3, 2022 21:47
@dairiki dairiki force-pushed the feature.ghp-publisher-refactor branch 3 times, most recently from 0dd7617 to eed0619 Compare February 28, 2022 04:20
dairiki added a commit to dairiki/lektor-website that referenced this pull request Feb 28, 2022
@dairiki dairiki force-pushed the feature.ghp-publisher-refactor branch from eed0619 to f921c78 Compare February 28, 2022 23:49
@dairiki dairiki marked this pull request as ready for review March 1, 2022 00:28
@yagebu
Copy link
Contributor

yagebu commented Mar 6, 2022

For ghpages, I think it's preferable to force-push an orphaned commit instead of having some history of deployed pages. Since that would be consistent with the current documentation (but not the implementation), I think changing it to work like that would be fine.

@dairiki
Copy link
Contributor Author

dairiki commented May 3, 2022

For ghpages, I think it's preferable to force-push an orphaned commit instead of having some history of deployed pages. Since that would be consistent with the current documentation (but not the implementation), I think changing it to work like that would be fine.

I tend to agree that force-push should be the preferred behavior, however, I worry about changing the default behavior without warning (other than in the changelog). In the rare cases where someone does want to preserve the history of the deployed HTML, changing to force-push will delete any such accumulated history.

The safer way to make the change would be to add a preserve_history=false option that would enable the force-push behavior. The default behavior would be left unchanged, but a warning would be issued that the default behavior will be changed in future and that users should add an explicit preserver_history=true if they want to keep the non-force-push behavior.

Is it worth the effort to do that?

It now supports:
- passing input to stdin
- capturing stdout
- returncode checking (both manual and automatic)
This cleans up the code, makes it more testable.

It also adds the ability to specify a branch name to which to
push (see lektor#978)

The GithubPagesPublisher tests have been removed from test_deploy.py
(New tests are in test_publisher.py.)
I can not figure out how to reliably test python response to an
external KeyboardInterrupt under Windows.  The response seems to
depend strong on which shell the python test process is run under.
(Under an interactive PowerShell, sending the python process either
a signal.SIGINT or signal.CTRL_C_EVENT seems to kill the shell.)
On python < 3.8, TemporaryDirectory fails when cleaning up dirs
containing read-only files on Windows.
@dairiki dairiki force-pushed the feature.ghp-publisher-refactor branch from f921c78 to dc5144b Compare July 22, 2022 14:58
Prefix the output from the git commands so that the (expected)
"fatal: ..." messages are not so jarring.
Also issue a DeprecationWarning announcing the intention to change
Lektor's default from 'master' to 'main' in the future.
@dairiki dairiki merged commit c7419cf into lektor:master Aug 2, 2022
@dairiki dairiki deleted the feature.ghp-publisher-refactor branch August 3, 2022 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github Pages: Change default branch from "master" -> "main"
2 participants