#committers
PR Reviews
PR review requests
A discussion thread where participants request reviews for pending Pull Requests (PRs) in FHIR and FHIR-extensions GitHub repositories. The goal is to unblock publication and ballot cycles. Specific PRs are linked, reviewers are tagged, and status updates are given. A side discussion emerges about build vulnerabilities caused by cross-repository dependencies.
pull requestreviewgithubpublicationbuildextensionsmerge
There are a few PRs pending review for both the fhir and fhir-extensions repositories. Knowing that there was a goal to publish the FHIR Extensions Pack IG on 4/1, just curious if those outstanding PRs will be reviewed soon?
For me, it is as follows, but several others appear to be waiting for review as well. PR #3049 (fhir) PR #3048 (fhir) PR #76 (fhir-extensions)
The only two people who can do that review are @Lloyd McKenzie and @Grahame Grieve
Poking helps. Reviewed
B
Any chance of getting this one reviewed: https://github.com/HL7/fhir-extensions/pull/69
B
We need it for publication of CQL IG and CRMI IG, both of which are dependencies of QM IG, which we are trying to get to ballot this cycle.
Reviewed
B
Thank you so much! But I'm a little confused by what change is being requested?
Nevermind. I somehow thought this was a PR against core, not extensions. Approved & merged
B
Great, thank you so much!
@Lloyd McKenzie I don't see any review comments on https://github.com/HL7/fhir-extensions/pull/76 ... Did that get reviewed/approved?
Sorry, only looked at ones in core. DOne
@Lloyd McKenzie can you review @Rob Hausam https://github.com/HL7/fhir/pull/3056 ? (I think his PR will fix the build errors I got when I merged master into my pending PR, https://github.com/HL7/fhir/pull/3048/commits . My PR built successfully until I merged master into it.)
B
Reviewed and merged, sorry, that's on me, I didn't realize when I changed the context of those extensions that it would break those examples in the main build :(
B
Thanks Rob for fixing!
This obviously shows a vulnerability for the main build that we may not have sufficiently recognized and considered? Not sure what might be best to do to minimize that risk. @Grahame Grieve @Lloyd McKenzie ?
this process is the best I can think of
Rob Hausam said : This obviously shows a vulnerability for the main build that we may not have sufficiently recognized and considered? Not sure what might be best to do to minimize that risk. Grahame Grieve Lloyd McKenzie ? Sorry to intrude. Gitflow is a process that prevents merge or version conflicts in open source teams. It provides stable versions, traceability and fast rollback.
it doesn't work between git repositories, which is where the problem arose here
Grahame Grieve said : it doesn't work between git repositories, which is where the problem arose here I understand. Sorry. In my opinion it's a fork.
@Lloyd McKenzie Can you review the following PR? J#43825 and J#44923 AllergyIntolerance.lastReactionOccurrence by michelle-m-miller · Pull Request #3064 · HL7/fhir (github.com)
Done
The changes we make to extensions shouldnt be breaking examples, as that's the experience users will also be getting. And an indicator that we've broken the version change rules in the extensions?
@Brian Postlethwaite - wrong thread?
Maybe a little late to the party, was more referring to extension changes breaking the build which was earlier in the conversation
@Lloyd McKenzie One more (small) PR pending your review. J#44689 - added Procedure property notes by michelle-m-miller · Pull Request #3072 · HL7/fhir (github.com)
@Lloyd McKenzie FYI - I have a couple PRs pending review. J#36009 AdverseEvent.status comments by michelle-m-miller · Pull Request #3083 · HL7/fhir (github.com) J#44834 Rename from CareTeam.participant.coverage to CareTeam.partici… by michelle-m-miller · Pull Request #3082 · HL7/fhir (github.com) J#45000 CarePlan.activity.performedActivity - updated description and… by michelle-m-miller · Pull Request #3081 · HL7/fhir (github.com)
@Lloyd McKenzie I also have a PR pending review FHIR-9197, FHIR-42819 TestScript updates and enhancements #3084
Reviewed
Thanks @Lloyd McKenzie -- and I have one more that just snuck in for review! J#43544 Clarified Goal statusDate and statusReason by michelle-m-miller · Pull Request #3085 · HL7/fhir (github.com)
Done
Pending PR review: https://github.com/HL7/fhir/pull/3087
@Jean Duteau Is there more review needed? It has been approved by Lloyd.
no review needed. I just need to accept it which I just did
@Jean Duteau I think the fhir core build is now broken
another PR to review... https://github.com/HL7/fhir/pull/3055
Here is an Imaging PR needing review/approval -- https://github.com/HL7/fhir/pull/3090
Here is a DocumentReference PR needing review/approval -- https://github.com/HL7/fhir/pull/3088
Here is a security/privacy PR needing review/approval -- https://github.com/HL7/fhir/pull/3089
thanks @Lloyd McKenzie ... I am updating security and docRef PR
J
OO extensions in need of review. Thanks! https://github.com/HL7/fhir-extensions/pull/114#partial-pull-merging
B
CRMI-related extensions for review please: https://github.com/HL7/fhir-extensions/pull/107
B
And 3 in support of the QICore ballot: https://github.com/HL7/fhir-extensions/pull/113 https://github.com/HL7/fhir-extensions/pull/112 https://github.com/HL7/fhir-extensions/pull/111
New PR in support of Public Health: https://github.com/HL7/fhir/pull/3108 ( @Lloyd McKenzie @Grahame Grieve )
can I get the above PR reviewed? ( @Lloyd McKenzie )
Done
B
@Lloyd McKenzie , changes to this extension PR based on the discussions on FHIR-I yesterday, I think this is ready for merge: https://github.com/HL7/fhir-extensions/pull/107
B
Also based on that discussion, the PR for the event-recorded extension: https://github.com/HL7/fhir-extensions/pull/112
B
@Michael Lawley on this PR, I made changes based on your suggestions, can you take a look and see if these address your concerns? https://github.com/HL7/fhir-extensions/pull/107 (and approve, it looks like the merge is blocked pending requested changes being made)
https://github.com/HL7/fhir/pull/3110 . ( @Lloyd McKenzie
@Lloyd McKenzie I have a couple PRs ready for review. https://github.com/HL7/fhir/pull/3120 https://github.com/HL7/fhir/pull/3123
@Lloyd McKenzie New PR for Nutrition updates: https://github.com/HL7/fhir/pull/3126
Lloyd is on holidays so I think we need @Grahame Grieve to approve PRs to core?
I'll get to them
New Nutrition PR that makes our value sets proper now that Grahame fixed the issue that was stopping us: https://github.com/HL7/fhir/pull/3142
I have 4 very small PRs for review. https://github.com/HL7/fhir/pull/3155 https://github.com/HL7/fhir/pull/3156 https://github.com/HL7/fhir/pull/3157 https://github.com/HL7/fhir/pull/3158 CC: @Lloyd McKenzie
Questions on two. Two approved.
I have several small PRs ready for review https://github.com/HL7/fhir/pull/3228 (Communication non-substantive updates) https://github.com/HL7/fhir/pull/3229 (add more Flag examples) https://github.com/HL7/fhir/pull/3231 (CareTeam non-substantive updates) https://github.com/HL7/fhir/pull/3232 (CarePlan boundaries and 5w updates) https://github.com/HL7/fhir/pull/3233 (Goal - substantive changes to remove outcome and rename individual to participant) https://github.com/HL7/fhir/pull/3234 (CommunicationRequest non-substantive boundaries update) CC: @Lloyd McKenzie
All done. (Grahame did one of them :smile: )
That was fast! Thank you both!
B
Extensions review for an approved tracker please: https://github.com/HL7/fhir-extensions/pull/134
valueSet active - https://github.com/HL7/fhir/pull/3247
John Moehrke said : valueSet active - https://github.com/HL7/fhir/pull/3247 can I get a PR review?
proposed PR to fix the build error. Please approve if this is good. https://github.com/HL7/fhir/pull/3249
DocumentReference jira tickets https://github.com/HL7/fhir/pull/3250
ImagingStudy and ImagingSelection jira tickets https://github.com/HL7/fhir/pull/3251
Security jira tickets https://github.com/HL7/fhir/pull/3252
@Bryn Rhodes - responsed on 134 @John Moehrke 3247 and 3249 approved. Others are failing the build
I will merge in the sex and gender fix to get the others to succeed.
3250 is ready to be approved
I need to fix something in 3251 so changed it to draft
3252 is building, but I do expect it to make it
3250 and 3252 approved
PR 3251 now ready for approval
#3253 ready for approval
3253 reviewed
I have an issue - I had a duplicate .contact element, and an issue with order of elements, now a searchparameter. but I don't see this in my local build. How can I do to see these errors locally?
example of error (the error is mine, I am fixing it) https://dev.azure.com/fhir-pipelines/fhir-publisher/_build/results?buildId=20751&view=logs&jobId=ab68b630-6476-573a-954f-d89e4292687e&j=ab68b630-6476-573a-954f-d89e4292687e&t=ab58c233-aa15-51d8-8258-3d2a9910e74a
my point is not to clog the online build infrastructure
B
@Lloyd McKenzie , responded on https://github.com/HL7/fhir-extensions/pull/134
@Bryn Rhodes Responded back :smile:
B
Okay, updated context for both extensions as requested
Approved
B
CDS extensions: https://github.com/HL7/fhir-extensions/pull/141
B
And two CQI extensions: https://github.com/HL7/fhir-extensions/pull/143 https://github.com/HL7/fhir-extensions/pull/139
Comments on 141. Others approved
B
Responded on 141
B
Also, I added a typo correction I noticed on the sdc fixes PR
B
The codeOptions extension, for preapplication: https://github.com/HL7/fhir-extensions/pull/128
Approved
I have 2 PRs for Extensions Pack IG (both deprecating extensions) pending review PR145 - https://github.com/HL7/fhir-extensions/pull/145 PR146 - https://github.com/HL7/fhir-extensions/pull/146
Approved
Review please on: FHIR-48741 - https://github.com/HL7/fhir/pull/3260 FHIR-48733 - https://github.com/HL7/fhir/pull/3261 FHIR-46510 - https://github.com/HL7/fhir/pull/3203
Approved
Can I get PR#3259 reviewed? https://github.com/HL7/fhir/pull/3259 (Very tiny change) I assume that means https://github.com/HL7/fhir/pull/3258 will also need to be reviewed as well since this is the fix for the build failure.
I have one more Extensions Pack PR for review (just adding more resources to context of use) if it isn't too late. https://github.com/HL7/fhir-extensions/pull/149
Requested changes
@Lloyd McKenzie made the change requested, so PR can be re-reviewed (but note that you moved on the J#40500 and supported the original resolution, but we can work through all that later)
One more PR to be reviewed (just removing instantiates to align with workflow pattern) https://github.com/HL7/fhir/pull/3265
Approved
@Lloyd McKenzie can you please re-review https://github.com/HL7/fhir/pull/3253 ? Build was broken so i had to fix it and preapply one other
Done
I am not sure if the requested change is editor discretion.
At this time of the day, I want to think it is.
It's a breaking change to the search parameter - different type and different semantic. Plus the name no longer applies to the content. So it seems like an error to me and a potentially dangerous one.
i see that, but this means: i should make the change? Or a Jira to make the change?
(nevermind)
(i need to reopen the Jira that was approved for this)
For now, maybe roll back that change and I can approve the rest of the PR?
that is the change - changing an element which was also in a search parameter. I think the only way I could roll back is if I remove the search parameter altogether for now.
Review please: https://github.com/HL7/fhir/pull/3270
https://github.com/HL7/fhir/pull/3272 . Thanks.
Jose Costa Teixeira said : that is the change - changing an element which was also in a search parameter. I think the only way I could roll back is if I remove the search parameter altogether for now. i actually missed that the parameter name needed to change per FHIR-49105 . So i did that now. @Lloyd McKenzie I think that satisfies your request, right?
y
https://github.com/HL7/fhir/pull/3253 can you re-re-review please?
Approved
D
Review please https://github.com/HL7/fhir/pull/3275
D
Review please https://github.com/HL7/fhir/pull/3273
First one approved, comments added to second
D
Lloyd McKenzie said : First one approved, comments added to second Responded but yes this was approved. @Marti Velezis can you comment here please
D
Post PR approval will you be merging in as well or can I trigger that?
Reviews please, for: https://github.com/HL7/fhir/pull/3272 https://github.com/HL7/fhir/pull/3271
Approved
David Barwin said : Lloyd McKenzie said : First one approved, comments added to second Responded but yes this was approved. Marti Velezis can you comment here please I posted the following on the PR requests: @lmckenzi and @grahamegrieve -- We we have been drafting module content for over a year for all of the healthcare product domains. We discussed this at the last couple of WGM with FHIR-I. We agreed to push content into the CI Build before @grahamegrieve decides how this should be integrated into the current modules. Grahame also said if we get push back on doing this to raise the issue -- so @grahamegrieve can we please get this moved to the CI as we agreed? If not, please let me know.
Integrating content into the current modules is not the same as defining a new module. I'm happy to merge the PR if Grahame wants the module page added before it gets integrated into the home pages and other places that modules get listed.
@Lloyd McKenzie This is the content we want to get out there to the community to make clear the use of resources in these domains. Whether it is decided that these are sub-pages to existing modules or new modules is still to be determined. We specifically asked if we could preview the content in the CI build without the decision and the answer was "yes". Waiting for @Grahame Grieve to reply.
Lloyd McKenzie said : Approved Thank you.
D
Few PRs piling up if reviewer is available, thank you!
I have a couple PRs coming today.... First one is https://github.com/HL7/fhir/pull/3285
Last couple PRs for review: https://github.com/HL7/fhir/pull/3286 https://github.com/HL7/fhir/pull/3287
Reviewed
fhir core build is broken. seems there has been some merges that were not approved?
B
I approved the one I merged, the PR build succeeded. Digging in to the failure it’s crashing on a tx call. I’ll see if I can bump it to get it fixed
https://github.com/HL7/fhir/pull/3296
https://github.com/HL7/fhir/pull/3298
Both approved
A
Molecular Sequence Resource: https://github.com/HL7/fhir/pull/3159
ActorDefintion and ExampleScenario: https://github.com/HL7/fhir/pull/3299
Reviewed
I have 3 small PRs for review https://github.com/HL7/fhir/pull/3302 https://github.com/HL7/fhir/pull/3303 https://github.com/HL7/fhir/pull/3304
Provided feedback.
@Lloyd McKenzie The JIRA resolution didn't include comments, so adding comments will require re-opening the JIRA. I added comments in hope that PC approves re-opening the JIRA. Can you re-review https://github.com/HL7/fhir/pull/3304 ?
@Lloyd McKenzie and the next one has comments added, too. https://github.com/HL7/fhir/pull/3302
Approved
The last one for today: https://github.com/HL7/fhir/pull/3303
Approved
Again: ActorDefinition and ExampleScenario: https://github.com/HL7/fhir/pull/3307
I have a couple more small PRs (adding Group in more places) https://github.com/HL7/fhir/pull/3331 https://github.com/HL7/fhir/pull/3332
@Lloyd McKenzie you had approved one of the above PRs, but not the other (I assume you were waiting on it to finish building?). It has built successfully now. https://github.com/HL7/fhir/pull/3332
Another PR for ActorDefinition: https://github.com/HL7/fhir/pull/3336 Thanks.
Both approved
PR from @Aly Khalifa that is ready: https://github.com/HL7/fhir/pull/3348
PR from @Patrick Werner that is ready: https://github.com/HL7/fhir/pull/3342
https://github.com/HL7/fhir/pull/3248
https://github.com/HL7/fhir/pull/3347 https://github.com/HL7/fhir/pull/3350
https://github.com/HL7/fhir/pull/3328
PR for moving a codesystem and valueset from ExampleScenario to ActorDefinition: https://github.com/HL7/fhir/pull/3354
https://github.com/HL7/fhir/pull/3356 ready for review
Approved
D
For review please: https://github.com/HL7/fhir/pull/3344
D
Another for review please: https://github.com/HL7/fhir/pull/3362
Approved
B
https://github.com/HL7/fhir/pull/3369 ready for review
https://github.com/HL7/fhir/pull/3365 ready for review
Approved
D
Please and thank you https://github.com/HL7/fhir/pull/3370
https://github.com/HL7/fhir/pull/3381 ready for review
https://github.com/HL7/fhir/pull/3386 ready
https://github.com/HL7/fhir/pull/3391 please
https://github.com/HL7/fhir/pull/3143 also please
https://github.com/HL7/fhir/pull/3390 please
when is deadline? I think today - at what time?
(have just 2 PRs giving time for any input that may arrive, otherwise we can merge)
B
Reconciliation deadline was yesterday, but the content freeze is the 18th
which is today
tomorrow is the 18th for me :smile:
Are we going by AEDT or EST? Today is still the 17th
ok. It's today-ish
it's not like a long way away
The biggest problem that I'm seeing is how long it takes to get an approved PR through the pipline of the automated checks before it can be merged. PR#3399 has been in the checks pipeline for the past 3 hours now, and I'm still waiting (and this was the second try, as the previous push was out of date by the time it finished the checks).
it doesn't matter if it's out of date unless the build fails
The content deadline is the date to allow review by work groups. Historically we've allowed changes after that date. Is there a reason we're not this time?
we've allowed a limited amount of changes after that driven by wg review, yes
I suspect several editors haven't even tried this time
https://github.com/HL7/fhir/pull/3392 pending review (after build)
https://github.com/HL7/fhir/pull/3407 ready for review
https://github.com/HL7/fhir/pull/3406 should be ready for approval soon (new nutrition examples)
so you will all see that the pipeline processors are not keeping up. :sad:
they're about 18 hours behind right now, taking about 1:15 for each run, and there's 2 of them, with 14 runs lined up, and PRs and merges are flowing faster than they can process
I have asked @David Otasek if we add more engines
https://github.com/HL7/fhir/pull/3405
All approved
https://github.com/HL7/fhir/pull/3404
D
Last time we had backlogs, I created two additional agents to handle it. These are now enabled again and handling jobs. We were only running a single agent until now.
my pharm PR build is failing with java.lang.Exception: Exception inserting section numbers in consent.html: Malformed XHTML: Found "</div>" expecting "</body>" at line 1755 column 12 at org.hl7.fhir.tools.publisher.Publisher.insertSectionNumbers(Publisher.java:6374) did I miss some updates? I know it built well locally, I could have missed / messed up something with branch sync
D
I think you may have done the opposite; master branch failed a build with the same error on its latest merge, and I think you likely have those changes in your branch (a few others also have this failure, FYI): https://dev.azure.com/fhir-pipelines/fhir-publisher/_build/results?buildId=22183&view=logs&j=ab68b630-6476-573a-954f-d89e4292687e&t=ab58c233-aa15-51d8-8258-3d2a9910e74a
hmm. That is a change I made to get rid of a warning, Did I mess it up? Just adding the required second <div> layer
too many close </div>
D
The offending merge appears to be this one: https://github.com/HL7/fhir/commit/07c798fa9ecd19d97611006c223bca7ed1fc8e7b
https://github.com/HL7/fhir/pull/3412
D
consent-introduction got a new </div> there.
@David Otasek did you see me just above admit to it, and provide a PR to fix?
D
Yes. You out-typed me while I was trying to finish my thought.
hmm, my PR failed too
D
That failed early, though, I don't think it even got to your change.
D
Huh.
D
And again. That's super weird.
D
Oh, sonatype is down.
D
Or was, for a heartbeat. I was getting 502 errors just trying to get to it. It couldn't get kindling from the maven repository there.
D
OK, NOW it seems to have picked up all the necessary parts: https://dev.azure.com/fhir-pipelines/fhir-publisher/_build/results?buildId=22186&view=logs&j=89ea4e08-a59f-59c8-d9a7-580121821729
so I rebase my PRs?
my PR has not yet been accepted. Once that is, then yes.
John Moehrke said : https://github.com/HL7/fhir/pull/3412 Urgent need to accept and merge else the master build won't succeed.
Approved
https://github.com/HL7/fhir/pull/3413 ready for review
Lloyd McKenzie said : Approved now just waiting for it's build to finish.. unless someone wants to force merge.
I don't have the power I'm afraid.
done. master should be back succeeding.
B
https://github.com/HL7/fhir/pull/3411 for review please, should be good once the build succeeds on that PR
I think https://github.com/HL7/fhir/pull/3414 is ready to merge
B
Craig Newman said : I think https://github.com/HL7/fhir/pull/3414 is ready to merge I think the mapping exceptions files shouldn't be committed with this change? The new SearchParameter definition looks good, but can you remove all the exceptions files from the commit since they aren't part of this change?
Agree. I'm not sure exactly what we're supposed to do with the mapping exceptions, but I haven't been committing them and they aren't part of this change. @Craig Newman
You should ideally be updating mapping exceptions for your own resources. It's not wrong to commit mapping exceptions that are unrelated to your change - all it'll do is add a bunch of 'unknown' divergences.
Ok. I think that makes sense. But it seems that many or probably most of us haven't been doing that regularly.
For what it's worth, I only made the search parameter update. GitHub added all of the other updates. If there is a way to separate them, that's fine with me. Will defer to the more knowledgeable people (which is pretty much everyone....)
PR#3415 is ready for a review.
@Craig Newman approved
@Rob Hausam approved
@Lloyd McKenzie - do I do the merge myself? Grahame did it for the last couple of pull requests I did.
B
If you have write privileges, you can merge it yourself once it's approved
Thanks @Bryn Rhodes - done
PR#3418 is ready for review.
Approved
PR#3420 is ready for review.
https://github.com/HL7/fhir/pull/3421 for review please
Approved
https://github.com/HL7/fhir/pull/3444 Ready for review
https://github.com/HL7/fhir/pull/3446 Ready for review
ok R6 ballot is closed. No more PRs
PRs ready for review: https://github.com/HL7/fhir/pull/3469 https://github.com/HL7/fhir/pull/3470
Another stack of PR ready for review: https://github.com/HL7/fhir/pull/3471 https://github.com/HL7/fhir/pull/3475 https://github.com/HL7/fhir/pull/3474
Another little stack of PR ready for review: https://github.com/HL7/fhir/pull/3477 https://github.com/HL7/fhir/pull/3478 https://github.com/HL7/fhir/pull/3479
Couple more PR ready for review https://github.com/HL7/fhir/pull/3480 https://github.com/HL7/fhir/pull/3481
PR ready for review : https://github.com/HL7/fhir/pull/3484
PR ready for review: https://github.com/HL7/fhir/pull/3508
Added comments
PR ready for review: https://github.com/HL7/fhir/pull/3518
@Lloyd McKenzie see my reply comments
Just waiting on the change to the PR for the XPath mime type
I noticed the fhirpath mime type was also wrong. Committed now.
I'm still seeing text/xml
Ok. I updated the StructureDefinition comments. You're seeing text/xml in the examples, right?
y
Fixing now...
Fixed and committed. Waiting for checks to complete.
Still seeing text/xml...
Where?
Never mind. The invariant-test files. On it.
Ok. Double-checked - all text/xml are now application/xml. Committed.
Approved
PR for approval https://github.com/HL7/fhir/pull/3522
PR for approval (Imaging) https://github.com/HL7/fhir/pull/3527
B
Extension PR please: https://github.com/HL7/fhir-extensions/pull/160
B
And another: https://github.com/HL7/fhir-extensions/pull/161
B
And another: https://github.com/HL7/fhir-extensions/pull/162
B
Thank you :)
Review please: https://github.com/HL7/fhir/pull/3530 . (Align DeviceAlert with 5Ws and Event pattern, address build warnings)
Another: https://github.com/HL7/fhir-extensions/pull/127 (Add Device-alertDetection extension)
https://github.com/HL7/fhir/pull/3532 - OO stuff fro priority backlog (Can I codename these? This one is is Petinga -young sardines. Several, can be eaten in one bite, not too much fat, nice if not too many or taken too often)
https://github.com/HL7/fhir/pull/3534 Ratfish (deep, weird, not worth it)
PR review please - https://github.com/HL7/fhir/pull/3537
B
Requested updates applied to https://github.com/HL7/fhir-extensions/pull/161 @Lloyd McKenzie
https://github.com/HL7/fhir/pull/3532 Still ready for review. Not that fresh anymore.
https://github.com/HL7/fhir/pull/3536 also ready
Approved
https://github.com/HL7/fhir/pull/3538
@Lloyd McKenzie
approved
Build is failing because of an instance of MedStatement. Due to this: https://tx.fhir.org/r5/CodeSystem/$lookup?system=http://www.nlm.nih.gov/research/umls/rxnorm&code=358793 on R4, it works https://tx.fhir.org/r4/CodeSystem/$lookup?system=http://www.nlm.nih.gov/research/umls/rxnorm&code=358793
(i assume the system is looking at r5 endpoint when building)
https://github.com/HL7/fhir-extensions/pull/173
https://github.com/HL7/fhir-extensions/pull/175
https://github.com/HL7/fhir/pull/3539 (fixes the med statement example errors in the core build)
Bump https://github.com/HL7/fhir-extensions/pull/165
Changes requested @Elliot Silver
https://github.com/HL7/fhir-extensions/pull/172 (new extensions Device mode, cycle and duration)
https://github.com/HL7/fhir-extensions/pull/166 (new device extensions)
https://github.com/HL7/fhir-extensions/pull/178 (new extension - SupplyDelivery)
B
PR for review please: https://github.com/HL7/fhir-extensions/pull/182
@Grahame Grieve I added context onto the relatesto-classifier extension that you added yesterday. https://github.com/HL7/fhir-extensions/pull/180
@Bryn Rhodes We are pre-applying the artifact-extended-contact-detail extension: https://github.com/HL7/fhir-extensions/pull/181
https://github.com/HL7/fhir-extensions/pull/183 (Added new extensions quantity-accuracy and quantity-confidenceInterval)
https://github.com/HL7/fhir-extensions/pull/184 (EndFhirVersion - Observation Precondition, Deprecated - ServiceRequest Precondition, and New extension (DiagnosticReport Interpretation Context)
https://github.com/HL7/fhir-extensions/pull/168 (endFHIRVersion device-maintenanceresponsibility)
https://github.com/HL7/fhir-extensions/pull/169 (Updated DiagnosticReport-Focus name)
https://github.com/HL7/fhir-extensions/pull/185 (updated extensions - deprecate, endFhirVersion)
https://github.com/HL7/fhir-extensions/pull/188 (QA fixes)
Can we get: https://github.com/HL7/fhir-extensions/pull/164 approved and merged?
Done
https://github.com/HL7/fhir-extensions/pull/189 (SDC extension changes)
DOne
https://github.com/HL7/fhir-extensions/pull/190 (More QA..)
Approved
D
Two reviews, minor text changes. Thank you. https://github.com/HL7/fhir/pull/3556 https://github.com/HL7/fhir/pull/3557
https://github.com/HL7/fhir/pull/3558 commented out no-longer-permitted extension
B
https://github.com/HL7/fhir-extensions/pull/193
D
Rosemary Hofstede said : https://github.com/HL7/fhir/pull/3558 commented out no-longer-permitted extension Looks like this PR and https://github.com/HL7/fhir/pull/3555 will address build issues. Please review
Approved, but it's not buildling properly
D
I think both of those PRs together are needed. Not sure next step - merge both into one to confirm build green?
D
David Barwin said : I think both of those PRs together are needed. Not sure next step - merge both into one to confirm build green? Confirmed green local build with these PRs merged, and I see @Jean Duteau has done the remote merge in https://github.com/HL7/fhir/pull/3558 :tada:
yep, it's just running a check and then I'll merge that in and we should all back to normal
B
PR, if there's still time: https://github.com/HL7/fhir-extensions/pull/194
One review, significant changes to Diagnostic Module page : https://github.com/HL7/fhir/pull/3562
B
Another extensions PR: https://github.com/HL7/fhir-extensions/pull/195
https://github.com/HL7/fhir-extensions/pull/196 (Added new extension - Observation Structure Laterality)
https://github.com/HL7/fhir-extensions/pull/197 (Added new extension - ServiceRequest Specimen Sugestion)
https://github.com/HL7/fhir/pull/3529 (Fix Nutrition examples)
An extensions PR: https://github.com/HL7/fhir-extensions/pull/200
https://github.com/HL7/fhir/pull/3569 (updated the example to remove deprecated extension)
https://github.com/HL7/fhir/pull/3568 (Add artifact-effectivePeriod usage notes and example to TestScript resource.)
https://github.com/HL7/JIRA-Spec-Artifacts/pull/1314 Added missing OO module to Jira Artifact
https://github.com/HL7/fhir/pull/3548 updated with new JIRA ticket
C
https://github.com/HL7/fhir-bfdr/pull/141 - oids.ini was updated when publisher was ran. Only manual change applied was relaxing cardinality on subject for composition
https://github.com/HL7/fhir-extensions/pull/205 (updated the extension description)
https://github.com/HL7/fhir/pull/3572 ( FHIR-51520 , 48690, 50567, 42947, 50612, 50132, 51494, 51531, 51530)
please review: Please review: https://github.com/FHIR/ig-registry/pull/449 (DE Medication IG)
Raman Dhanoa said : https://github.com/HL7/fhir/pull/3572 ( FHIR-51520 , 48690, 50567, 42947, 50612, 50132, 51494, 51531, 51530) @Lloyd McKenzie It looks like this was approved and merge process started and it's stuck now: image.png
Simplest might be to pull master into your branch and re-push and see if that gets things working again. If not, then close and re-issue the PR and ping me on Signal to re-approve.
there are a bunch of PRs that are stuck with the "Pull Request Pipeline (buildAndPublishSpecification)" check waiting for status to be reported: #3577, #3575, #3574, #3573, #3571, It looks like the last PR to successfully pass the build checks was #3570 4 days ago
As an example, this is showing as "in progress", even though the pipeline shows as complete @Josh Mandel @Grahame Grieve @Joshua Procious do we know what's happening with the pipeline?
J
I am investigating but I do not currently know what is happening.
do we have any update on this? It is blocking all PRs to the core spec. @Josh Mandel maybe you have deeper knowledge of what might be going on?
J
The only thing I can contribute is this was the last successful PR build check: https://github.com/HL7/fhir/pull/3566 . I'll wait for Josh as I suspect this may be on the Azure side
Please review: https://github.com/HL7/fhir/pull/3585 (FHIR- 40749, 49750, 51646)
My PR to add the german medication ig to the ig registry is still open: Please review and merge: https://github.com/FHIR/ig-registry/pull/449
https://github.com/HL7/fhir/pull/3607 ( FHIR-50529 TestScript constraint updates and re-org)
A
https://github.com/HL7/fhir/pull/3554 is ready for review, thanks in advance
https://github.com/HL7/fhir/pull/3613 is ready for review
https://github.com/HL7/fhir/pull/3614 is ready for review
https://github.com/HL7/fhir/pull/3616 is ready for review
https://github.com/HL7/fhir/pull/3618 is ready for review
https://github.com/HL7/fhir/pull/3622 ready for review, thanks
All approved
typo PR https://github.com/HL7/fhir/pull/3624 ready for review
loinc display changes https://github.com/HL7/fhir/pull/3631 ready for review
https://github.com/HL7/fhir/pull/3627 https://github.com/HL7/fhir/pull/3628 https://github.com/HL7/fhir/pull/3629 Thank you.
PR ready https://github.com/HL7/fhir/pull/3636
PR for updating the german package feed url ready for review and merge: https://github.com/FHIR/ig-registry/pull/461
thanks for the fast merge, unfortunately my PR contained the wrong url (publication instead of package url. Fixing this error: https://github.com/FHIR/ig-registry/pull/463 Sorry @Grahame Grieve
PR ready https://github.com/HL7/fhir/pull/3639 that fixes some less than successful expressions from 3636
https://github.com/HL7/fhir/pull/3652 ready for review with some minor pharmacy updates
https://github.com/HL7/fhir/pull/3659 ready for review
https://github.com/HL7/fhir/pull/3664 - new DeviceAlert example.
@Lloyd McKenzie PR for JIRA Spec file for Molecular Definition Implementation Guide for Molecular Data Types IG: https://github.com/HL7/JIRA-Spec-Artifacts/pull/1348
reminder ping for the several PRs that are ready
@Lloyd McKenzie
Responded to the non-draft, non-ancient ones
https://github.com/HL7/fhir-extensions/pull/209 - this is on the extensions pack.
Thank you.
I have a few PRs ready for review https://github.com/HL7/fhir/pull/3691 https://github.com/HL7/fhir/pull/3692 https://github.com/HL7/fhir/pull/3693 https://github.com/HL7/fhir/pull/3694
Another extension pack PR https://github.com/HL7/fhir-extensions/pull/210 please. Also, https://github.com/HL7/fhir/pull/3690 https://github.com/HL7/fhir/pull/3695
Some nutrition updates: https://github.com/HL7/fhir/pull/3696
More PRs coming today (note: Grahame said he was working on fixing the unrelated build failure in #committers > Another terminology build error? ) https://github.com/HL7/fhir/pull/3694 https://github.com/HL7/fhir/pull/3697 https://github.com/HL7/fhir/pull/3698
One last PR (for today, at least!) https://github.com/HL7/fhir/pull/3699
Elliot Silver said : Another extension pack PR https://github.com/HL7/fhir-extensions/pull/210 please. Also, https://github.com/HL7/fhir/pull/3690 https://github.com/HL7/fhir/pull/3695 @Lloyd McKenzie , when you get a moment...
Reviewed
Thank you.
Just one PR for review today! https://github.com/HL7/fhir/pull/3704
J
Extension pr (workflow stuff) for review. Thanks! https://github.com/HL7/fhir-extensions/pull/208
PR (small) on DocumentReference event pattern update https://github.com/HL7/fhir/pull/3712
and security module page -- https://github.com/HL7/fhir/pull/3713
When you all put in PR that update a Resource, please also include the updated event and fivews mapping from your local build; preferably with them having no "unknown" rational. There are many resources for which a build have changes in these files, and it is not proper for the resource owner to have not submitted their changes. Current resources with outdated mappings: adverseevent device devicedispense diagnosticreport familymemberhistory guidanceresponse supplydelivery supplyrequest
John Moehrke said : PR (small) on DocumentReference event pattern update https://github.com/HL7/fhir/pull/3712 approve this pr... please?
Ready for review: https://github.com/HL7/fhir-extensions/pull/212
B
Extension PR please: https://github.com/HL7/fhir-extensions/pull/213
B
FHIR PR please: https://github.com/HL7/fhir/pull/3722
Reviews please: https://github.com/HL7/fhir/pull/3739 (thanks @David Barwin ) https://github.com/HL7/fhir/pull/3738 https://github.com/HL7/fhir/pull/3737 https://github.com/HL7/fhir-extensions/pull/215
P
https://github.com/HL7/fhir/pull/3741
PRs ready for review: https://github.com/HL7/fhir/pull/3743
Ready for review: https://github.com/HL7/fhir/pull/3742
https://github.com/HL7/fhir/pull/3744 https://github.com/HL7/fhir/pull/3746 https://github.com/HL7/fhir/pull/3747 https://github.com/HL7/fhir/pull/3748 https://github.com/HL7/fhir/pull/3749 https://github.com/HL7/fhir/pull/3751 https://github.com/HL7/fhir/pull/3752
Ready for review: https://github.com/HL7/fhir/pull/3754
P
Please review https://github.com/HL7/fhir/pull/3755
Ready for review: https://github.com/HL7/fhir/pull/3756
Ready for approval: https://github.com/HL7/fhir/pull/3729
Ready for review: https://github.com/HL7/fhir/pull/3758
B
Bump on this extensions PR please: https://github.com/HL7/fhir-extensions/pull/213
B
And on this FHIR PR: https://github.com/HL7/fhir/pull/3722
D
Review please https://github.com/HL7/fhir/pull/3732
P
Please review https://github.com/HL7/fhir/pull/3761 .
P
Also, # FHIR-20153 - Remove duplicate text from comments #3758 is saying 1 check required - is the build tooling stuck or is this something else?
For https://github.com/HL7/fhir/pull/3758 , try pushing a whitespace change to see if it retriggers.
P
Please review https://github.com/HL7/fhir/pull/3763
Please review: https://github.com/HL7/fhir/pull/3762
P
Please review https://github.com/HL7/fhir/pull/3765
I have a couple PRs ready for review https://github.com/HL7/fhir/pull/3768 https://github.com/HL7/fhir/pull/3769
Suggestion on the first, both approved.
P
Please review https://github.com/HL7/fhir/pull/3770
P
Please review https://github.com/HL7/fhir/pull/3772
P
Please review https://github.com/HL7/fhir/pull/3773
3772, approved but suggested correction
P
3772 had a build error so I deleted, replacement below Please review https://github.com/HL7/fhir/pull/3774
Please review: https://github.com/HL7/fhir/pull/3775
P
Please review https://github.com/HL7/fhir/pull/3776
Please review https://github.com/HL7/fhir/pull/3778 fyi @Bryn Rhodes
P
Please review https://github.com/HL7/fhir/pull/3780
P
Please review https://github.com/HL7/fhir/pull/3781
P
Please review https://github.com/HL7/fhir/pull/3783
P
Please review https://github.com/HL7/fhir/pull/3785
P
Please review https://github.com/HL7/fhir/pull/3787
Please review https://github.com/HL7/fhir/pull/3782
More please, https://github.com/HL7/fhir/pull/3794 .
Please review https://github.com/HL7/fhir/pull/3797 (fhirpath page in the fhir spec)
B
It looks good to me, but also, the diffs are showing as complete replacements for the html files, is that a formatting things? It makes it pretty much impossible to tell what actually changed?
Please review https://github.com/HL7/fhir/pull/3792
Please review https://github.com/HL7/fhir/pull/3798 Note: includes a fhir.ini update that builds correctly but I'm not 100% sure is correct.
@Jonathan Whitby , requested changes
P
Please review https://github.com/HL7/fhir/pull/3796
Lloyd McKenzie said : Jonathan Whitby , requested changes Thanks. Updated fhir.ini as requested. Waiting for pipeline to run now.
Bryn Rhodes said : It looks good to me, but also, the diffs are showing as complete replacements for the html files, is that a formatting things? It makes it pretty much impossible to tell what actually changed? You could look at each of the commits which are only covering a single issue (or part thereof) if you wanted finer details. Yes there was a lot of changes... Quite the backlog I got through. Oh, and you can hide whitespace changes and things get much clearer...
P
Please review https://github.com/HL7/fhir/pull/3802
P
Please review https://github.com/HL7/fhir/pull/3805
P
Please review https://github.com/HL7/fhir/pull/3807
Please review: https://github.com/HL7/fhir/pull/3811 @Bryn Rhodes
B
PR for review please: https://github.com/HL7/fhir/pull/3809
B
@David deRoode , a few minor tweaks on that one
P
Please review https://github.com/HL7/fhir/pull/3806
B
@Paul Knapp , comments on the PR
P
@Bryn Rhodes we have a later ticket, to be applied at the end, which will set all extensible and many example bindings to preferred so I am not setting the bindings as per the ticket just to undo it later.
B
Got it, can you please make that as an editorial comment on the tracker?
Bryn Rhodes said : David deRoode , a few minor tweaks on that one Updates made- two minor clarifications outstanding
P
@Bryn - comment made.
P
Please review https://github.com/HL7/fhir/pull/3813
B
@David deRoode , reviewed, basically, you need to nest the parameters, not use the . notation, so instead of: <part> <name value= "component" /> ... </part> <part> <name value= "component.code" /> ... </part> It needs to be: <part> <name value= "component" /> ... <part> <name value= "code" /> ... </part> </part>
for https://jira.hl7.org/browse/FHIR-50489 , it is .stratifier.component.code. So stratifier is the <parameter>. See the screenshots in the PR that demonstrate nested multi-part failing to rendering correctly (therefore, dot notation as a workaround). @Bryn Rhodes
B
@Paul Knapp , on 3813, see: https://hl7.org/fhir/searchparameter-definitions.html#SearchParameter.target
P
Please review https://github.com/HL7/fhir/pull/3815
P
@Bryn re 3813, on it
B
@David deRoode , right, component.code is in the <part> element of the stratifier parameter, it needs to be in the <part> element of the component parameter. At least based on the screenshots that are there, they aren't nested properly (and looking at what's committed).
B
Happy to jump on a call if that would help?
Got it (<part> within <part>). Fixed. Thanks @Bryn Rhodes
P
@Bryn re 3813 changes applied
B
@Paul Knapp , in the expression is good, but also, after processingMode, add a target element: <target value= "Patient" />
P
@Bryn Rhodes FHIR-43122 EnrollmentRequest and EnrollmentResponse added targets to candidate search parameters
D
Review please https://github.com/HL7/fhir/pull/3800 https://github.com/HL7/fhir/pull/3795
(deleted)
Please review https://github.com/HL7/fhir-extensions/pull/218
Please review https://github.com/HL7/fhir/pull/3819
please review https://github.com/HL7/fhir/pull/3821
@Lloyd McKenzie - I have a very small PR (non-substantive change) for review. https://github.com/HL7/fhir/pull/3823
P
Please review https://github.com/HL7/fhir/pull/3825
Paul Knapp said : Please review https://github.com/HL7/fhir/pull/3825 @Paul Knapp did you fix your build problems before merging? It looks like the build is now failing?
P
@John Moehrke buildsfine locally, trying to see where the issue(s) are.
P
@john and I haven't merged .
P
@John Moehrke - I have deleted 3825 as the ticket is redundant with FHIR-43309 (PR 3820) which currently needs a review and may be what's holding up the queue.
P
Please review https://github.com/HL7/fhir/pull/3828
Please review https://github.com/HL7/fhir/pull/3830 @Bryn Rhodes fyi
P
Please review https://github.com/HL7/fhir/pull/3831
P
Please review https://github.com/HL7/fhir/pull/3827
P
Please re-review https://github.com/HL7/fhir/pull/3820
Please review https://github.com/HL7/fhir/pull/3824 . Note: This fixes all ImagingSelection event mapping exceptions but ImagingStudy still claims to have one, despite no evidence of it in the generated imagingstudy-event-mapping-exceptions.xml .
B
Please review: https://github.com/HL7/fhir/pull/3841
@Bryn Rhodes , small fix needed
B
I see your comment, but github isn't showing me what you commented on so I'm not sure what part isn't valid? I got a successful full build locally and in the ci build, so I'm not sure what to fix
image.png
No clue why the build isn't failing. It should... @Grahame Grieve ?
D
Review please: https://github.com/HL7/fhir/pull/3832
Reviews please: https://github.com/HL7/fhir/pull/3844 https://github.com/HL7/fhir/pull/3843
R
Please review: https://github.com/HL7/fhir/pull/3839
why should it fail?
R
Please review: https://github.com/HL7/fhir/pull/3845
R
Please review: https://github.com/HL7/fhir/pull/3845
B
@Lloyd McKenzie was the issue the missing description of the binding? If so, I have fixed that, please re-re-review: https://github.com/HL7/fhir/pull/3841/
https://github.com/HL7/fhir/pull/3846 - Nutrition Updates
Review please: https://github.com/HL7/fhir/pull/3848
Jean Duteau said : https://github.com/HL7/fhir/pull/3846 - Nutrition Updates Why is this pull request failing? It says that the image is missing which is the whole point of the pull request. I can’t see any other file where the image is referenced.
Please review https://github.com/HL7/fhir/pull/3850 , this replaces PR#3820 due to how many files were changing and merge conflicts to bring the rest of the code up to date.
Jean Duteau said : Jean Duteau said : https://github.com/HL7/fhir/pull/3846 - Nutrition Updates Why is this pull request failing? It says that the image is missing which is the whole point of the pull request. I can’t see any other file where the image is referenced. Did you add the image to publish.ini?
I don't see anything in that PR referencing the image either. And you shouldn't have build.fhir.org references, I don't think.
Yes. The point is to delete the image and I removed the one place it was referenced. I’ll keep looking to see what other file might be referencing it
Please review: https://github.com/HL7/fhir/pull/3851 (might need to ignore whitespace in a few files that look like its a file replacement - it's not) https://github.com/HL7/fhir/pull/3851/files?diff=unified&w=1
R
(deleted)
Please review https://github.com/HL7/fhir/pull/3847 (note that this deletes some unused value set files, I haven't ever deleted files as part of a Pull Request. I think I did it right, but please double check)
D
Extension for review: https://github.com/HL7/fhir-extensions/pull/219
sigh, i made a formatting problem, please review this PR: https://github.com/HL7/fhir/pull/3854
Please review PR: https://github.com/HL7/fhir/pull/3857
@David Barwin Question asked
D
Lloyd McKenzie said : David Barwin Question asked Responded. There was considerable committee debate here and it may evolve in future.
Approved, though shared some of my opinions :slight_smile:
R
Please review: https://github.com/HL7/fhir/pull/3855
Review please: https://github.com/HL7/fhir/pull/3858
B
Though I think the elements in the SearchParameter are out of order
P
Please review https://github.com/HL7/fhir/pull/3861
@Paul Knapp Requested a correction
P
@Lloyd McKenzie Removed date as a choice.
B
Please review: https://github.com/HL7/fhir/pull/3864
Please review: https://github.com/HL7/fhir/pull/3865
P
lease review https://github.com/HL7/fhir/pull/3861
please review https://github.com/HL7/fhir/pull/3866
D
Small extension update review: https://github.com/HL7/fhir-extensions/pull/220
@Brian Postlethwaite Changes requested
@Chris Cioffi Changes requested
Lloyd McKenzie said : Chris Cioffi Changes requested Moved the constraints onto the element. Please review PR. Thanks! https://github.com/HL7/fhir/pull/3866
@Chris Cioffi What I'm asking for is to declare the Condition pointing to your constraint on those elements whose appearance or value is governed by your constraints. So I'd expect to see conditions on paymentBy and insured for both constraints
Lloyd McKenzie said : Chris Cioffi What I'm asking for is to declare the Condition pointing to your constraint on those elements whose appearance or value is governed by your constraints. So I'd expect to see conditions on paymentBy and insured for both constraints Oh! I'll figure that out this afternoon. Thanks for clarifying.
This is different that the PR above. Please review: https://github.com/HL7/fhir/pull/3869
Chris Cioffi said : Lloyd McKenzie said : Chris Cioffi What I'm asking for is to declare the Condition pointing to your constraint on those elements whose appearance or value is governed by your constraints. So I'd expect to see conditions on paymentBy and insured for both constraints Oh! I'll figure that out this afternoon. Thanks for clarifying. please review, I think this addresses everything: https://github.com/HL7/fhir/pull/3866
https://github.com/HL7/fhir/pull/3873 Thank you.
P
Please review https://github.com/HL7/fhir/pull/3874
Because of the tx.fhir.org change, the Evidence resource examples on master FHIR build are now failing. I made a fix to the examples by changing the CodeSystem used. If urgent, you can merge into the master build without the content check, or move it to the first in the queue. The Pull Request: https://github.com/HL7/fhir/pull/3876
P
@Khalid Shahin I don't have anything to do with the Evidence resource.
I didn't mean it as a response.
P
@Khalid Shahin Do you know which PR it relates to?
The change that is making the FHIR build fail isn't within the FHIR repo, but from a CodeSystem ( https://fevir.net/sevco ) being added to the terminology server. Now that has been added, the examples that have been using that CodeSystem for a while are now failing within the FHIR build because the codes aren't loaded yet. So I made a PR that changes the codesystem url to something else to get the FHIR build to work for now. The FHIR validator was pointing out that the specific codes were invalid, updated them to valid codes.
P
Got it.
P
@Khalid Shahin still failing
Yep, I still need to merge my PR. The check failed near the end because of the tx server. Running it again.
Nope, now it's producing errors like this related to tx.fhir.org : Summary: Errors=565, Warnings=4480, Information messages=1956 ERROR @ MedicationAdministration.dosage.route (line 35, col6): medicationadministration0308:: MedicationAdministration.dosage.route: Error performing tx5 operation 'validate-code: Connect timed out' (parameters = "") ERROR @ MedicationDispense.dosageInstruction[0].route (line 55, col6): medicationdispense0316:: MedicationDispense.dosageInstruction[0].route: Error performing tx5 operation 'validate-code: Connect timed out' (parameters = "") ERROR @ MedicationRequest.dosageInstruction[0].route (line 48, col6): medicationrequest0320:: MedicationRequest.dosageInstruction[0].route: Error performing tx5 operation 'validate-code: Connect timed out' (parameters = "") ERROR @ NutritionIntake.nutritionItem[4].type (line 215, col6): nutritionintake-example:: NutritionIntake.nutritionItem[4].type: Error from https://tx.fhir.org/r5: Unparseable HTML Source (Malformed XHTML: Found "</body>" expecting "</hr>" at line 6 column 9) ERROR @ NutritionIntake.nutritionItem[4].nutritionProduct.concept (line 223, col8): nutritionintake-example:: NutritionIntake.nutritionItem[4].nutritionProduct.concept: Error from https://tx.fhir.org/r5: Unparseable HTML Source (Malformed XHTML: Found "</body>" expecting "</hr>" at line 6 column 9) ERROR @ NutritionIntake.nutritionItem[4].notConsumedItem[0].reason (line 232, col8): nutritionintake-example:: NutritionIntake.nutritionItem[4].notConsumedItem[0].reason: Error from https://tx.fhir.org/r5: Unparseable HTML Source (Malformed XHTML: Found "</body>" expecting "</hr>" at line 6 column 9) (truncated) https://dev.azure.com/fhir-pipelines/fhir-publisher/_build/results?buildId=26905&view=logs&j=89ea4e08-a59f-59c8-d9a7-580121821729&t=6b1df179-88b1-5a56-f023-406ef5304c18&l=3107
server looks ok
P
The codes are invalid - I have included valid placeholders in my build.
The tx server was timing out an hour ago part way through but it looks good now. I'll have my PR pass the check and merge so the Evidence examples no long cause the build to failure.
Never mind it's still failing: https://dev.azure.com/fhir-pipelines/fhir-publisher/_build/results?buildId=26915&view=logs&j=ab68b630-6476-573a-954f-d89e4292687e&t=ab58c233-aa15-51d8-8258-3d2a9910e74a&l=3107 Will see if Paul's PR passes the check.
B
That seems transient, I actually got that same error in an IG build, bumped it and it went away
D
Any updates to build?
It's building now.
I had to make a new branch and now it's working. It kept failing 26 minutes in no matter how many times I'd re-run the checking on Azure. It was working locally though. https://dev.azure.com/fhir-pipelines/af4e28d8-13e5-41ea-9d9f-f6c9e34fb19f/_apis/build/builds/26953/logs/13 So the solution is to make a new branch if you get errors like this " ERROR @ MedicationAdministration.dosage.route (line 35, col6): medicationadministration0308:: MedicationAdministration.dosage.route: Error performing tx5 operation 'validate-code: Connect timed out' (parameters = "") "
Please review https://github.com/HL7/fhir/pull/3883 fyi @Bryn Rhodes
B
There's a comment on measure identity parameters
Bryn Rhodes said : There's a comment on measure identity parameters resolved
B
Thank you
please review: https://github.com/HL7/fhir/pull/3887
@Lloyd McKenzie PR ready for review: https://github.com/HL7/fhir/pull/3890 Note: The change was fairly small (binding strength change and removal of invariant con-2), but the diff is misleading -- it looks like a bigger change since the removal of con-2 caused con-3/4 to be renamed to con-2/3 and the invariant test files all shifted as well.
you don't have to rename the invariants. it's better to maintain continuity with previous versions
OK, let me start over and see if a simple delete of con-2 works. Local build was erroring out, but might have been due to the invariant tests... hold on this PR for a sec.
@Lloyd McKenzie Please ignore the last PR above, and review this new one instead https://github.com/HL7/fhir/pull/3892 (in this version, I didn't re-number the invariants)
Thanks!!
Three Pull Requests: Structured Documents: https://github.com/HL7/fhir/pull/3767 Group.characteristic: https://github.com/HL7/fhir/pull/3464 Study Design ValueSet that uses the SEVCO CodeSystem listed with THO, and we will need to remove the Example ValueSet/CodeSystem files that are no longer used after this PR: https://github.com/HL7/fhir/pull/3889
https://github.com/HL7/fhir/pull/3896 is finally building, will add the missing examples and recheck in a few hours
Elliot Silver said : https://github.com/HL7/fhir/pull/3873 Thank you. Re-review please. Added new content.
https://github.com/HL7/fhir/pull/3898 ready for review
Please review https://github.com/HL7/fhir/pull/3900 fyi @Bryn Rhodes
B
Just a few minor tweaks please
D
Jose Costa Teixeira said : https://github.com/HL7/fhir/pull/3896 is finally building, will add the missing examples and recheck in a few hours This PR is now green with examples so I'd guess ready for review
thank you @David Barwin . indeed, https://github.com/HL7/fhir/pull/3896 ready for review
QA fixes in security wg domain - https://github.com/HL7/fhir/pull/3905
@Jose Costa Teixeira changes requested
@John Moehrke changes requested
@Lloyd McKenzie I had these to apply FHIR-31632 FHIR-51726 FHIR-50142
are they wrong and should be reverted?
should I preapply https://jira.hl7.org/browse/FHIR-53305 ?
Code systems must be in core if they are: code systems with experimental=true (which can only be bound as example), and code systems bound to elements with data type 'code'. Everything else needs to be in THO.
My inclination on 53305 is 'yes'.
I'm pretty sure @Grahame Grieve would agree the element is not a modifier
On CodeSystems: what do you think is the best way to unblock the PR? Should I add the note or make them experimental?
personally, my take on the codesystems would be to make one single (experimental?) codesystem for medrequest, statement, dispense (they are the same values, and it's a category, it's not something we want to be too directive about). but that would require a committee discussion
FHIR-53305 preapplied. Will check with committee monday to see if we have to change something.
D
Smaller change now sounds like a comment. There are a bunch of valuesets needing attention in coming weeks
@Lloyd McKenzie I have this ready to preapply too if that makes the PR acceptable https://jira.hl7.org/browse/FHIR-53306
https://github.com/HL7/fhir/pull/3911 will preapply the issue. If that is agreeable, then this PR makes the bit Pharmacy PR mergeable
please review https://github.com/HL7/fhir/pull/3910
I don't think the categories are intended to be experimental. Those look like 'real' values to me, and I'm surprised to not see them at least as preferred. It would be odd to define those codes and say "but you're not allowed to use them". I do agree that it's reasonable to define a single code system and value set.
My recommendation would be to define a single non-experimental code system with a note that it'll be moving to THO - because I don't think you have enough time to make that happen.
B
Yeah, and those are already in THO and published with that binding for all those resources as of R5, so I don't know what change needs to be made?
B
I guess the committee is saying they ought to be example?
B
But for MedicationRequest at least, US Core is using the codes in THO.
B
Please review https://github.com/HL7/fhir/pull/3914/
Category code are examples always/commonly
I made the PR within the boundaries of what the working group decided.
Should i revert that and unapply the issue to allow the entire PR to merge? Or go back to the working group.to revisit the decision?
@Jean Duteau may remember why we moved the codes out of THO
B
Yes, I can see it was within what the committee decided, but maybe they weren't aware that US Core is using these codes in a non-example way?
B
And it looks like all the codes are the same, regardless of what resource they are being used in, so I'd suggest the WG make example bindings to the THO medicationrequest category value set, and just remove all the terminology from the base spec. Same effect, but keeps them all consistent with medication request, and with current real-world usage in at least US Core. And if US Core is doing it there are probably others (I know the current CMS and HEDIS measures use those codes too)
I agree we should fix that. I could not use the same codesystem because the same codes change their definition slightly ("includes orders in ..." vs "Includes administrations in"...
we should change the designations if we want to do that.
at this moment I see two possible actions for now: patch the applied issue(s) somehow, or revert the move. both will require fixing later
It's possible to have an "example" binding strength to non-experimental codes. If the work group really wants this to be an example rather than preferred binding, that's ok. But the codes certainly seem appropriate for real-world use, so they shouldn't be experimental - and thus should live in THO. (Consolidating the duplicate code systems into a single code system is also appropriate.)
We have a call tomorrow and can resolve that. But for today and for the huge PR to be merged, what do you think I should do?
keep experimental false, add the the comment? That is what https://github.com/HL7/fhir/pull/3911 does
For now, I think you're too late to move to THO, so just flag the code systems and say they'll be moved and likely consolidated. Don't mark them as experimental. You can keep the binding strength as example. (I don't understand why that and not something tighter, but that's a committee decision) The comment about things moving needs to be a ballot note in the intro to the code systems, not just on the bound elements
ok will update the PR with the ballot comments in the code systems
Have we come up with a particular rule or pattern about having a ballot note on every code system (and value set?) that is going to move to THO, but hasn't been moved there yet?
is there an example of a ballot note in a codesystem? How do I do that?
The med request category is in THO and we got told to take it out of THO by TSMG
B
Right, if the committee said it was example content, TSMG would say it shouldn't be in THO. But it's being used in production by at least US Core, so we're suggesting the committee should not consider it example content and it should remain in THO.
@Rob Hausam We don't have a policy for value sets yet - and them moving to THO is a non-substantive change. @Jose Costa Teixeira As best I can tell, there unfortunately seems to be no support in core to have an 'introduction' on a code system, so I guess you'll have to put the relevant warning in the introduction to the resources that bind to the code system. In your intro, add a blockquote element with the class "ballot-note". You can see an example in observation-introduction.xml.
i got the ballot note but I was indeed unable to add it to the code system intro. Will add ballot notes to the resources that have a category and are expected to change. Will also put some bold text in the description of each codesystem
@John Moehrke Approved
in the resource: image.png in the valueset: image.png
is this ok?
yes
nice
https://github.com/HL7/fhir/pull/3896 ready for review then
thank you all for the input
please review https://github.com/HL7/fhir/pull/3912
B
Please review https://github.com/HL7/fhir/pull/3916
B
@Rik Smithies , approved, but with some minor suggestions
thanks Bryn I was not aware of the target attribute
https://github.com/HL7/fhir/pull/3917 ready (was actually applied in a previous PR, but for some reason it doesn't show in the build). I presume it is ok to apply as a TC, even if operation is normative
minor text changes missing for closing jiras: https://github.com/HL7/fhir/pull/3918 I hope it is still possible to review these.
Please review: https://github.com/HL7/fhir/pull/3924 example update as per jira FHIR-48914
please review https://github.com/HL7/fhir/pull/3921
I need a review for a fhir-extensions PR, please. https://github.com/HL7/fhir-extensions/pull/214
Thank you, and this one as well: https://github.com/HL7/fhir-extensions/pull/216
Please review https://github.com/HL7/fhir/pull/3930 (the fhirpath fhir page issues)
B
A minor tweak
B
Bumping review on this one please: https://github.com/HL7/fhir/pull/3916
@Bryn Rhodes Made some comments
Saw them, he's approved.
GG found a typo, and I've also pre-applied another ticket which has been waiting for some time. (the outstanding part of the ticket was where to put the additional guidance, and that should go in the incubator, but the property change is solid)
B
@Lloyd McKenzie , I thought that was searchParameters, I've seen resolve() in other constraints, that's why I thought it was okay: https://build.fhir.org/observation.html#invs
B
(deleted)
B
I'd be happy to remove those constraints, I'm a little worried about enforcement performance anyway, but then I can only enforce that with narrative?
I really think that the relatedArtifact .resolve() constraints are very difficult. When would resolve() ever work? So you tried to use relatedArtifact.predecessor, you'd need to ensure that the predecssor is in scope. Like a package dependency? So everything depends on everything?
Also, this constraint: type in ('successor' | 'predecessor') implies ((resource.resolve() is PlanDefinition) or (artifact.resolve() is PlanDefinition)) this fails if resolve() fails. As it usually will
I mean, maybe I don't care, because it'll just ensure that this bad feature of relatedArtifact is never used
B
Right, I'll remove the constraints and just state them narratively in the comments on the element.
I think that this is right, and I'll do the same with FHIR-50011
B
Changes applied, re-review please: https://github.com/HL7/fhir/pull/3916
One minor typo @Bryn Rhodes
B
Review please: https://github.com/HL7/fhir/pull/3932
hey @Bryn Rhodes why did you delete conceptmap-cdshooks-indicator?
B
It was part of the CDS Hooks mapping that we agreed should move to the CDS IG
B
I should say "is" part of that mapping
which IG?
B
https://build.fhir.org/ig/HL7/cds-hooks-clinical-reasoning
which task was this?
I have a PR for FHIR-52870 CDSHooks Indicator to Request Priority map should not be example, and also FHIR-48308 ConceptMaps SHOULD specify used CodeSystems. Did you get the change from 48308?
B
https://jira.hl7.org/browse/FHIR-53085
B
Nope, I didn't know about those, but I will apply both those to the ig
ok thanks
B
https://build.fhir.org/ig/HL7/cds-hooks-clinical-reasoning/branches/main/en/ConceptMap-cdshooks-indicator.html
https://github.com/HL7/fhir/pull/3935 Sorry, just realized a typo - My bad - if not possible to merge, I understand.
Please review https://github.com/HL7/fhir/pull/3936
https://github.com/HL7/fhir/pull/3933 is ready for review
and so is https://github.com/HL7/fhir/pull/3937
Please review https://github.com/HL7/fhir/pull/3938 fry @Bryn Rhodes
Please review https://github.com/HL7/fhir/pull/3939 (has FHIR-49784 )
B
@Grahame Grieve a few comments in 3933 and one that I think isn't quite right based on the tracker disposition as I read it?
B
@David deRoode , we need to either bring the library in or change the expressions to in-line so there's no library required
@Bryn Rhodes good catches, thanks. fixed
B
Question on 3937, SupplyRequest and SupplyDelivery are out but the StructureDefinitions are still there?
Both of those should move out — @Grahame Grieve can you fix that?
I said that I left the source behind for your reference, but they're not used anywhere
Oh - sorry - got it.
B
Right, then approved :)
thanks
B
Ready for review: https://github.com/HL7/fhir/pull/3941
Ready for review: https://github.com/HL7/fhir/pull/3940
B
Ready for review: https://github.com/HL7/fhir/pull/3943
Ready for review https://github.com/HL7/fhir/pull/3949
https://github.com/HL7/fhir/pull/3947 @Grahame Grieve
https://github.com/HL7/fhir/pull/3951 also ready to confirm again (previous PR https://github.com/HL7/fhir/pull/3926 was approved but failed because I gave the branch a bad name and that didn't go well)
The new publisher is updating alot of files, and you can't run it twice. Has anyone else noticed that? On second run I get FHIR build failure @ Thu, Nov 6, 2025 10:17-0600 0.742 22sec 1119MB Error: Cannot invoke "String.startsWith(String)" because the return value of "org.hl7.fhir.r5.model.Identifier.getValue()" is null java.lang.NullPointerException: Cannot invoke "String.startsWith(String)" because the return value of "org.hl7.fhir.r5.model.Identifier.getValue()" is null at org.hl7.fhir.definitions.validation.ValueSetValidator.getOid(ValueSetValidator.java:287) at org.hl7.fhir.definitions.validation.ValueSetValidator.validate(ValueSetValidator.java:209) at org.hl7.fhir.tools.publisher.Publisher.generateValueSetsPart1(Publisher.java:6956) at org.hl7.fhir.tools.publisher.Publisher.loadValueSets1(Publisher.java:1781) at org.hl7.fhir.tools.publisher.Publisher.execute(Publisher.java:809) at org.hl7.fhir.tools.publisher.Publisher.main(Publisher.java:588) Unless I revert all the changes the publish caused on first run.
PR Ready for review https://github.com/HL7/fhir/pull/3952
"new publisher" -- do you mean kindling or IG Publisher? I saw something like this last night building core. I thought I'd done a bad pull.
This is FHIR core... publish
Approved with comment
https://github.com/HL7/fhir/pull/3951/ - change was already approved but CI was not working due to my poor choice when it comes to branch names. Had to cancel the other PR and create a new one
please review https://github.com/HL7/fhir/pull/3950
@Jose Costa Teixeira Question on https://github.com/HL7/fhir/pull/3947
@Lloyd McKenzie replied
Extension pack review please https://github.com/HL7/fhir-extensions/pull/221 (has all approved PA extension changes in it) And extensions pack is now building again
B
Review please: https://github.com/HL7/fhir/pull/3955
B
@Brian Postlethwaite approved with questions
PR ready for review https://github.com/HL7/fhir/pull/3956
Ready for review: https://github.com/HL7/fhir/pull/3958 FML grammar update
there's 3 PRs on this page https://github.com/HL7/fhir/pulls from @Corey Spears . None of them will queue up for a build - does anyone have any idea what's going on?
can someone please approve https://github.com/HL7/fhir/pull/3959
please review https://github.com/HL7/fhir/pull/3960
One very small, non-substantive PR ready for review https://github.com/HL7/fhir/pull/3961
@Grahame Grieve , has the issue been figured out? Was it something I did?
https://github.com/HL7/fhir/pull/3967 is ready for review and based of the right now current master branch :-)
https://github.com/HL7/fhir/pull/3967 is ready for review and based of the right now current master branch :-) I just looked and added some comments. I'm not brave enough to approve that monster though.
https://github.com/HL7/fhir/pull/3965 is ready for review - just fixing an incorrectly-created invariant from a previous ticket.
Elliot Silver said : https://github.com/HL7/fhir/pull/3967 is ready for review and based of the right now current master branch :-) I just looked and added some comments. I'm not brave enough to approve that monster though. I think I addressed most of the comments. I have an idea on the 'Resource interval tests:', but I cannot resolve it right now but need to stew on it a bit more. I think that I need to change the section to always represent two tests and clarify whether they are 'and' or 'or', but I am failing to figure out a clear representation.
I’ll try to look at it again tonight. By the way, your PR is a massive effort, and greatly appreciated.
Thanks, and thank you - reviewing is also a massive effort, and equally as appreciated :-)
please review https://github.com/HL7/fhir/pull/3968
Please review https://github.com/HL7/fhir/pull/3972 (minor PA QA fixes)
@Rik Smithies can you look at https://github.com/HL7/fhir/pull/3908 please
Can you please review this extension pull request, please? https://github.com/HL7/fhir-extensions/pull/216
can someone approve this one please: https://github.com/HL7/fhir/pull/3973
If it's not too late: https://github.com/HL7/fhir/pull/3975
that's the last one.
B
If it can't make it, totally understand, apologies, I was trying to get this one in, but I had other things to do this morning and couldn't get to it: https://github.com/HL7/fhir/pull/3976
B
Super simple, just the addition of EventDefinition to relativeTime that we reviewed yesterday in FHIR-I
Please review this PR: https://github.com/HL7/fhir/pull/3982 Content okayed by Grahame in advance
@Grahame Grieve here's the PR for the QA issue discussed https://github.com/HL7/fhir/pull/3983
@Rik Smithies @Raman Dhanoa @JD Nolen @Reinhard Egelkraut @Aly Khalifa @Bas van den Heuvel , you all have old PRs against the base spec that I'm going to close. If you think that's wrong, feel free to bring them back to life
and can someone approve this please: https://github.com/HL7/fhir/pull/3985
just tidying up notes to balloters
Approved.
R
Grahame Grieve said : Rik Smithies Raman Dhanoa JD Nolen Reinhard Egelkraut Aly Khalifa Bas van den Heuvel , you all have old PRs against the base spec that I'm going to close. If you think that's wrong, feel free to bring them back to life Hi @Grahame Grieve , It was decided in PA that he content, that my PR is addressing, is now part of the Additional Resources space. Is it better to just close the current PR and open a new one or is there a way to “migrate” the PR so that it can be applied against one of the incubator IGs?
No you’ll have to apply it again
R
got it, thanks
please review https://github.com/HL7/fhir/pull/3986 (agreed with Grahame)
can someone approve https://github.com/HL7/fhir/pull/3987/files please
@Grahame Grieve to what tickets do you refer? The list popping up using the following query ( https://jira.hl7.org/issues/?jql=project%20%3D%20FHIR%20AND%20Specification%20%3D%20%22FHIR%20Core%20(FHIR)%20%5BFHIR-core%5D%22%20AND%20Reporter%20%3D%20bvdh%20and%20status%20not%20in%20(%20Applied%2C%20Duplicate%2C%20%22Resolved%20-%20No%20Change%22%2C%20Published) ? In this list there are a large number of tickets that have been voted on but that are not yet applied. Some became obsolete based on recent specification updates. That leaves a set that I think need to be addressed in the specification. I understand the need to publish R6, but I object to closing these topics . Marking them as Defferred will work. That allows me to review them in December, close those that are no longer relevant and include the others in my ballot comments.
@Bas van den Heuvel I said nothing about the jira tasks. Only that the PR was way out of date, and coulid not be merged. I said: If you think that's wrong, feel free to bring them back to life so feel free to do that. I have said nothing about the tasks not being worth applying
Ready for review : https://github.com/HL7/fhir/pull/3995
Ready for merging: https://github.com/HL7/fhir/pull/4010
@Craig Newman I don't see a list of changes made in the notes file for the resource?
@Lloyd McKenzie I added a list of updates to the Pull Request. Is there a specific Notes file that needs to be updated?
It should be the resource intro for changes to a resource, or at the top of a page if you're changing content not related to a resource.
@Lloyd McKenzie I added the Notes to Balloters for both Immunization and the Medications Module
Security PR ready for review https://github.com/HL7/fhir/pull/4028
@John Moehrke Please flag the changes made at the top of the security and signature pages and in the Provenance intro
@Lloyd McKenzie What kind of a change is note worthy? I changed "can't" to "can not".
If the changes are only technical corrections, then that's fine. Please flag in the PR if that's the case.
I am following the example from immunization. I presume that is the right pattern?
PR 4028 ready for another review
D
Extension pack PR for review please - https://github.com/HL7/fhir-extensions/pull/227
D
Core PR for review please - https://github.com/HL7/fhir/pull/4006
Presume OO plans to change from just references to th trackers to more descriptive text before publication?
D
Yes absolutely!
D
PR for review: https://github.com/HL7/fhir/pull/4029
D
Extension PR for review: https://github.com/HL7/fhir-extensions/pull/228
Extension PR for review: https://github.com/HL7/fhir-extensions/pull/224
D
Extension PR for review please: https://github.com/HL7/fhir-extensions/pull/229
D
Extension PR for review: https://github.com/HL7/fhir-extensions/pull/230 There is also this one that has been sitting awhile but looks okay to me? https://github.com/HL7/fhir-extensions/pull/222
Extension PR for review please: https://github.com/HL7/fhir-extensions/pull/231
222 has issues. We shouldn't be deprecating the extension.
D
Sounds good will take back to committee
Extension PR for review please: https://github.com/HL7/fhir-extensions/pull/232
And another extension PR for review please: https://github.com/HL7/fhir-extensions/pull/233
D
Core change for review please - https://github.com/HL7/fhir/pull/4030
D
Extension PR for review: https://github.com/HL7/fhir-extensions/pull/235
D
Core PR for review: https://github.com/HL7/fhir/pull/4031
Please review this FHIR Core PR: https://github.com/HL7/fhir/pull/4032
D
David Barwin said : Extension PR for review: https://github.com/HL7/fhir-extensions/pull/235 @Lloyd McKenzie please see question in the PR thank you
D
David Barwin said : Core change for review please - https://github.com/HL7/fhir/pull/4030 @Lloyd McKenzie can you merge these too? Looks like I've lost that ability
Done, though it's weird that you've lost merge privileges. @Grahame Grieve , did we change the rules?
I don't know
D
Maybe something left over from ballot freeze?
D
Not sure if other folks are seeing the same, I was added to a few FHIR IGs relatively recently, maybe something broke with that
I believe all HL7-controlled repositories now have branch protections turned on, so if you are (for example) trying to push to main or master (whatever the default branch is) that will be blocked. You need to work in another branch and use a PR workflow. (not sure if that applies here without more context, but would be my first guess)
this is mostly not true for IGs. I have some vague memory of FMG deciding to do this for the FHIR spec
Really? I thought that was turned on for everything now.. nvm I guess.
it shouldn't be. It imposes a real performance hit to the editorial process, and that isn't always justified
For some IGs, there's only one person doing the work, so independent review of commits isn't realistic.
(Many, in fact)
also, we don't have a useful build framework for IGs other than looking at the output on build.fhir.org
I was not referring to the PR review (which I did not think for anything other than the fhir repo) - I was referring to branch protections which prevent pushing directly to the default branch, which is pretty typical to avoid 'oops'.
I don't really follow. the only oops I'm familiar with is committing to the default branch when I meant to follow the PR workflow
Yes? That is exactly what that protection prevents (if turned on). It prevents the "I thought I was in a branch and pushed these changes" errors, which is why I thought the default policy had it turned on.
but there's no point doing all this for most IGs; people don't work in branches and don't have PR workflows
In IHE I defined a rule... during pre first draft, no branch protection. But once first draft then a PR with a second review was necessary. I get the point that it is not always easy for a team to have two git users, but it does seem to align best with standards governance.
D
That issue is not what I am running into though with FHIR Core - I've always had to work in a branch, but once that PR was approved, I was previously able to merge my PR. Today I can create the branch and PR but cannot merge that PR after approval. Change must have been in the last 3 months or so around R6 ballot cutoff.
Jonathan Whitby said : Please review this FHIR Core PR: https://github.com/HL7/fhir/pull/4032 @Lloyd McKenzie : Thanks. I believe I have addressed your comment.
Core change for review please (Genomics Guidance page): https://github.com/HL7/fhir/pull/4034
Again, I will make a plea for clear, detailed and accurate documentation on FHIR update processes for those of us who are not GitHub experts. I find it very confusing (AKA error prone) that editing FHIR core, creating IGs and updating Jira Spec Artifact all seem like very different processes. For example, you fork in GitHub for Jira Spec Artifact but not for other types of updates. It's not clear who approves PRs (or when they are needed). Building core locally uses "publish.bat" where building an IG uses "genonce.bat" and Jira Spec Artifact uses "buildlocal.bat". There may be very good reasons for the differences, but for those of us who don't do this all day, every day, it's very hard to be sure we are doing it right (and ensuring that we are minimizing the amount of time we spend bothering all of you when things go wrong).
D
Please merge this approved core PR so it can be QA'd: https://github.com/HL7/fhir/pull/4031
Agree with @David Barwin here, I used to be able to merge PRs once they were approved, but I can no longer merge, so something changed with the access rights on the FHIR core repository.
https://github.com/HL7/fhir/pull/4008 ready
Provided feedback