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.

Sign up for GitHub

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

Jump to bottom

Adds a local copy of the npm packages for the validator #3241

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Adds a local copy of the npm packages for the validator #3241

wants to merge 8 commits into from

Conversation

feordin
Copy link
Contributor

@feordin feordin commented Apr 19, 2023

Description

Add a local copy of the NPM packages for the validator.

Related issues

Addresses [issue #].

Testing

Describe how this change was tested.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@feordin feordin requested a review from a team as a code owner April 19, 2023 00:20
@feordin feordin added New Feature Label for a new feature in FHIR OSS Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs labels Apr 19, 2023
@feordin feordin added this to the S113 milestone Apr 19, 2023
@feordin feordin added the Enhancement Enhancement on existing functionality. label Apr 19, 2023
LTA-Thinking
LTA-Thinking reviewed Apr 21, 2023
View reviewed changes
src/Microsoft.Health.Fhir.R4B.Api/Microsoft.Health.Fhir.R4B.Api.csproj Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.R5.Api/Microsoft.Health.Fhir.R5.Api.csproj Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.R5.Web/Microsoft.Health.Fhir.R5.Web.csproj Show resolved Hide resolved
src/Microsoft.Health.Fhir.Stu3.Api/Microsoft.Health.Fhir.Stu3.Api.csproj Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.Shared.Core/Features/Validation/ProfileValidator.cs Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.Shared.Core/Features/Validation/ProfileValidator.cs Show resolved Hide resolved
src/Microsoft.Health.Fhir.Shared.Core/Features/Validation/ProfileValidator.cs
throw;
}
}

internal static (string Server, string CorePackageName, string ExpansionsPackageName) GetFhirPackageVariables()
{
Type type = typeof(FhirPackageSource);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this coming from? I can't find reference to this FhirPackageSource or where its fields are defined.

Copy link
Member

Choose a reason for hiding this comment

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

These are compiled into the Firely libraries, it isn't a great dependency, but its a way to find the NPM urls. Perhaps we should define these ourselves?
There is a unit test to cover they exist: https://github.com/microsoft/fhir-server/pull/3241/files#diff-98805c9eb649d4044301a52d69bb13b1e09768a441ded93ce851277986c8103aR18

brendankowitz
brendankowitz reviewed Apr 21, 2023
View reviewed changes
src/Microsoft.Health.Fhir.R4.Web/Microsoft.Health.Fhir.R4.Web.csproj
Comment on lines +15 to +23
<Content Include="hl7.fhir.r4.core-4.0.1.tgz">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
<Content Include="hl7.fhir.r4.elements-4.0.1.tgz">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
<Content Include="hl7.fhir.r4.expansions-4.0.1.tgz">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
Copy link
Member

Choose a reason for hiding this comment

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

Should we be checking these into OSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question. Previously we did reference the specification.zip file, which was part of the nuget that we were using. This is the same content, but in this case we are including the package in our repo.

brendankowitz
brendankowitz reviewed Apr 21, 2023
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ValidateTests.cs Outdated
public async void GivenAValidateByIdRequest_WhenTheResourceIsValid_ThenAnOkMessageIsReturned()
{
Skip.If(ModelInfoProvider.Instance.Version == FhirSpecification.R5, "Validation not currently working correctly for R5");
Copy link
Member

Choose a reason for hiding this comment

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

A little scared of doing this. It isn't an R5 thing, its a Firely package thing. Can we skip if Hl7.FHIR packages <= version x.x.x?

@feordin
Copy link
Contributor Author

feordin commented Apr 24, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@feordin
Copy link
Contributor Author

feordin commented Apr 25, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@feordin
Copy link
Contributor Author

feordin commented Apr 26, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@feordin
Copy link
Contributor Author

feordin commented May 1, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@feordin
Copy link
Contributor Author

feordin commented May 10, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LTA-Thinking LTA-Thinking LTA-Thinking left review comments

@brendankowitz brendankowitz brendankowitz left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality. New Feature Label for a new feature in FHIR OSS
Projects
None yet
Milestone
S113
Development

Successfully merging this pull request may close these issues.
3 participants
@feordin @brendankowitz @LTA-Thinking