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

Fixed #8 in submodlue metaschema-xslt to fix the cross-site linking #10

Conversation

JustKuzya
Copy link
Contributor

Resubmitting the previously done and approved job

@JustKuzya JustKuzya requested a review from a team as a code owner August 7, 2023 19:29
Copy link
Contributor

@aj-stein-nist aj-stein-nist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked wendellpiez/metaschema-xslt and usnistgov/metaschema-xslt on the develop branch and everything lines up, modulo the PR I reviewed and merged with Dmitry. I am happy with this, but will leave for awareness, understanding, finally review by other team members before releasing.

@wendellpiez
Copy link

wendellpiez commented Aug 8, 2023

I will need to be instructed on whether and how this gets merged into the submodule source.

As for the patch itself, this might better be handled by adjusting $reference-link rather than mapping it in place. See usnistgov/metaschema-xslt#59. This actually raises an interesting question of regression testing the Hugo build.

The bottom line is that link checking probably always needs to be run over generated files as presented, regardless.

@aj-stein-nist
Copy link
Contributor

aj-stein-nist commented Aug 8, 2023

I will need to be instructed on whether and how this gets merged into the submodule source.

The code was merged into metaschema-xslt, we are discussing this on the PR in OSCAL-Reference to push up the metaschema-xslt submodule in this repo.

As for the patch itself, this might better be handled by adjusting $reference-link rather than mapping it in place. See usnistgov/metaschema-xslt#59. This actually raises an interesting question of regression testing the Hugo build.

I reviewed the issue and understand the high-level relationship to the PR that was merged and we want to move up here, but is there something in particular we want to address in this submodule update?

The bottom line is that link checking probably always needs to be run over generated files as presented, regardless.

Totally agreed. As a course of approving usnistgov/metaschema-xslt#57, I tested it by locally adjusting the submodule and trying several of the links (if one changed correctly, so do they all) before working with Dmitry to create this PR. Do you recommend any immediate changes or fixes in sprint? I don't disagree with your comments and circling back on it, but I am not sure you mean it has to happen now or later. Let us know.

EDIT: Sorry, I pointed to the wrong metaschema-xslt issue, not the actual PR Dmitry made.

@wendellpiez
Copy link

From what you say, all sounds good:

How to maintain and regression test the Hugo-generation capability with the others (since links are one thing that commonly changes between deployments) is something we'll need to do with that Issue, which I will note in a comment.

@aj-stein-nist aj-stein-nist merged commit 7446f58 into usnistgov:main Aug 8, 2023
@aj-stein-nist
Copy link
Contributor

From what you say, all sounds good:

* The right thing is now being done for Hugo

* The correction is already in the upstream repo

* We can pick up the alignment with standalone production in [Test, adjust and plan refinements to standalone Docs production metaschema-xslt#59](https://github.com/usnistgov/metaschema-xslt/issues/59)

How to maintain and regression test the Hugo-generation capability with the others (since links are one thing that commonly changes between deployments) is something we'll need to do with that Issue, which I will note in a comment.

Sounds good then, I merged the PR.

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.

3 participants