-
Notifications
You must be signed in to change notification settings - Fork 9.1k
Update schema-publish.sh #4376
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
Merged
+56
β28
Merged
Update schema-publish.sh #4376
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
This command seems to have been lost. It is necessary when building to
deploy/
for thelinks on https://spec.openapis.org to work.
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 saw that command, but couldn't reason what it was doing, since no
.md
files are generated in/deploy/**
.Running the script with a clean or missing
/deploy
makes it a no-op.Can you point to me where the link-updating bit happens, or show me where the
.md
files make their way to/deploy
?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
*.md
files only exist in thegh-pages
branch, for example in https://github.com/OAI/OpenAPI-Specification/tree/gh-pages/oas/3.1/schema.The
title
of each file is specific to that file, and I didn't want to hard-code them in the build script to minimize the differences between the OpenAPI, Overlay, and Arazzo copies of the build script.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'll re-add this command, but I think it indicates a weird space in the abstraction. It seems contradictory to me to both
mkdir -p deploy/oas/3.1.1
and to expect that there are already markdown files present there.I also think it's a bad pattern to do a
mv
where the input could be multiple files, but the destination is a single file.It looks like the file we expect to move is actually named the same as $date. Short of restoring
latest
, could I just move that one$date.md
file?Alternatively, we could go back to having a
latest.md
, but on publish, rename it to match the latest iteration of the schema. This would preserve the publishing behavior (not creating alatest
link or schema), while still removing step of manually renaming the.md
file with each revision and allowing us to reference a stable filename for copying in this script.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've pushed a change which replicates the current
*.md
behavior, as well as introduces some commented-out code probing the pain points mentioned in my comment above.π One implementation supposes the
.md
file's name always matches the iteration date, which matches the current setup, and is still simpler than globbing files.π One implementation supposes the
.md
file is alwayslatest.md
. This used to be the setup, but it was abandoned. I think this approach is best. See below for more articulation.π One implementation mirrors the current behavior, but selects only the "first" markdown file encountered. This aligns better with the sentiment A
mv
command whose destination is a file should have a file as its first argument, but in a kind of nonsense way.π The actual implementation mirrors the current behavior, attempting to copy all the markdown files in the deploy directory to $target.md. If there are multiple markdown files present this results in an error later in the script. The actual implementation mirrors the previous one with the
head -n 1
removed, to highlight its similarities to that approach.π
latest.md
is my favoriteThe implementation which supposes a markdown file
latest.md
will be present is my favored approach.πΏ indicates a benefit to the current process
π indicates a benefit we already have, first gained by abandoning
latest
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 discussion, please move it over to