Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Request for comments]: Site Versioning #1870
base: master
Are you sure you want to change the base?
[Request for comments]: Site Versioning #1870
Changes from 8 commits
4eb63a8
e180361
c229160
e64f91c
778f3cb
95298d4
8eed90c
8453f7a
1ba8c91
f113c22
0769bc7
aca406b
7f559b1
792c156
4fc9090
798d63f
b85641a
9beec61
ad5f0ff
601d343
b52a341
039c0bd
f394060
c77b95e
5f8e427
2362390
3337749
46632f1
5ef1776
850aa18
d6cc776
a4505ce
f8ee5d7
1bdb924
3745f7a
d89e2a6
7d8125d
14fe6ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should we put
archivePath
into a named parameter instead? (e.g.-p --archive-path
)Since we don't expect that most people would need to change this
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.
Good idea, I agree with this! Will do so unless anyone else disagrees.
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.
Maybe can throw an error here so that it will stop the archiving process if the version name is not specified?
Currently, it will be archived as
undefined
: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.
wonder if we should add in
archive [root] <versionName>
to make things consistent with the other commands (cliUtil.findRootFolder
), but it should be backward compatible in any case so we can decide on this later.I think minimally we could add in a small note in the cli commands page that the site archived is the
cwd
's one.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.
should we? in view of all the other stuff in there
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.
(as opposed to the regeneration ones)
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 think missed this part too (implementation is inside this function)
#1870 (comment)
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.
Mostly looks like on the right track, I think the biggest missing thing may be:
I realised with this note, perhaps we could also store the
baseUrl
used to archive the version (if we ever want to support changing baseurls) insideversions.json
, as differing baseUrls shouldn't be copied over to_site
.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'm not sure what you mean by "differing baseUrls sholudn't be copied over to
_site
" - from my understanding, the HTML files do away with references to baseURL by replacing references to baseURL with the baseURL. But yes, I can definitely store the archived site's baseUrl!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 believe for now, if we do a
markbind build
after the archive command, the archived HTML files will be copied over to the_site
folder (which assumes that they have the samebaseUrl
as the main site). If the archived sites have a differentbaseUrl
, then their HTML files should not be copied over to_site
folder since they are probably not going to be part of the same deployment.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.
Hmm, I see...
It makes sense that differing baseUrls shouldn't be copied over (as they wouldn't be able to be deployed), but I wonder if there are any cases where the baseUrls might differ, but you might still want to include it as part of the same deployment and it would work (something like a root baseUrl being 'root' and the subsite baseUrl being "root/subsite" - it still could be part of the same deployment (?)).
I think copying it over is not a significant cost or a bug, and if the user wants to it is easy to manually exclude by indicating that folder as a folder to ignore. But it shouldn't be too hard to exclude the files as well 🤔
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.
Subsites are meant to be entire sites able to be deployed independently.
Our support for subsites is more about content reuse https://markbind.org/userGuide/reusingContents.html#reusing-contents-across-sites, in particular revolving around resolving
{{baseUrl}}
properlyE.g.
Main site, baseUrl is '/cat'
Subsite , baseUrl is '/dog'
subsite index.md
If you deploy the subsite independently,
baseUrl
resolves to/dog
.If you deploy the main site,
baseUrl
resolves to the main site base url + sub folder (i.e./cat/subfolder/of/subsite
).This is important wherever there are links involved (link validaiton, relative link resolving,
<include>
s, ...).If I'm getting this part right (ignore as in the
ignore
property), the versioning system's copying mechanism needs to be independent of asset copying (theignore
property).e.g. if you have
but at the point of archiving,
xx.png
wasn't present (meaning the author did indeed want to copy it over). The copying for versioning should still copyxx.png
over.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.
The point on
ignore
however is this: If you had archived a site, say intoversions/v1
. Then, you ranmarkbind build/serve
. MarkBind's asset copying would incorrectly / incompletely copy some parts of it over (since it is in the project source folder). This copying mechanism therefore needs to be implemented separately of this. The asset copying should also exclude said archive folders.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.
@ang-zeyu Oh :0 So the desired default behaviour, is to serve only the current site, and not any archived sites, because of the cost in copying over large sites? That makes sense. I had assumed the opposite, which is that if the user had archived the site previously they would want to check it when building and serving.
Currently, my understanding/implementation is:
versions.json
- previously archived versions will not be re-archived._site
are wiped clean, and replaced by the newly built files + copied over assets (which now include the versioned sites)._site
and only those contents are transferred togh-pages
branch --> this will include all versioned sites which were not specifically ignored by the user.And what I understand from your suggestion is that:
_site
_site
which hold versions._site
clean, and will need to store the names of subfolders to "ignore"(either not overriding with versioning or not serving with markbind serve)(One clarifying question: does this mean you suggest to store the archived site two places, both in
_site
and in the folder<archivePath>/<versionName>
?)I can give it a shot, but do you think I can leave this as an improvement for another PR? I think it might be a little tricky because it affects big commands and features and might take more than a few days to implement carefully (might also make reviewing easier)
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.
Nope, nothing so complex, ignore the additional point on
markbind serve
first, I'll come back to that later.Your current understanding is mostly already correct.
This is the main issue, it is incorrect to rely on asset copying for this. #1870 (comment)
You'll have to implement an independent copying mechanism.
If you need convincing, try archiving our documentation with
markbind archive v1
. Then, addappveyorAddNewProject.png
toignore
, and runmarkbind build
. The image will be missing from the archived site inversions/v1/images
.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.
*Please make sure you get the above comment before reading this comment, otherwise it'll only be more confusing
Back to this, my concern here was just with this:
markbind serve
3.1 Triggers your copying mechanism everytime (file change/addition/deletion). This is rather disk intensive / expensive and might slow the development experience a fair bit.
Not in the way you are thinking, but I realised with your comment you'll also need to add the versioned site folders to the
pagesExclude
property here.https://markbind.org/userGuide/siteJsonFile.html#pagesexclude
This is as:
.md
files may be copied to output folders as an asset in some projectsBack to @jonahtanjz's comment here as well, you'll need to take note of this when implementing the copying as well. (i.e. don't copy archived sites with different baseurl from the current one in
site.json
)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.
Thanks for explaining that again, and for the simple example - that's really useful to test my thoughts.
I hadn't realized that we "supported building html files as pages". I had the understanding that "building" a page was converting it from
.md
to a generated.html
- will take a closer look at this.