Skip to content

Refactor testingbranch workflow#23

Merged
ryma2fhir merged 15 commits intomainfrom
refactor-testingbranch-workflow
Nov 10, 2023
Merged

Refactor testingbranch workflow#23
ryma2fhir merged 15 commits intomainfrom
refactor-testingbranch-workflow

Conversation

@declankieran
Copy link
Copy Markdown
Contributor

I've removed all the poetry stuff here and I've also change the install of node to use an action.

The python requirement is to do with the

https://github.com/NHSDigital/validation-service-fhir-r4/blob/main2/scripts/update_manifest.py

(is this supposed to be main2?)

This updates this manifest file

https://github.com/NHSDigital/validation-service-fhir-r4/blob/main2/src/main/resources/manifest.json

Guessing this is used in the build. This branch is currently in a state where its not calling this.

I can add it back in without poetry and call it, the only package dependency is requests and pip is available in python now. I personally think it's hiding something very useful, i.e. the ability to just specify a package. As it is, it will always do the latest. I would imagine there are cases where it'd be useful to try stuff with older releases, and in the current state you'd need to remove a step in an action if you wanted to apply an update to the manifest.

@ryma2fhir
Copy link
Copy Markdown
Contributor

ryma2fhir commented Oct 16, 2023

Can you remove the pyproject.toml file as well? This is no longer needed as was being used only by Poetry.

@ryma2fhir
Copy link
Copy Markdown
Contributor

Regarding Manifest.json - https://github.com/NHSDigital/validation-service-fhir-r4 is EPS's validation service. Ours is https://github.com/NHSDigital/IOPS-FHIR-Validation-Service, so we shouldn't be updating theirs. They are using the main branch, not main2, so removing it won't upset their install.

It is used in the validation repo, but has been updated manually by KM as and when we have a new UKCore / NHSE package. Whether we would want manual control over which package we would like to validate against is another thing. I think we would, but we could always add a ticket to automate it and decide another day.

@ryma2fhir
Copy link
Copy Markdown
Contributor

Would be worth adding the maven cache to this as well #25

@declankieran
Copy link
Copy Markdown
Contributor Author

It is used in the validation repo, but has been updated manually by KM as and when we have a new UKCore / NHSE package. Whether we would want manual control over which package we would like to validate against is another thing. I think we would, but we could always add a ticket to automate it and decide another day.

This is the manifest that is in main2

https://github.com/NHSDigital/validation-service-fhir-r4/blob/main2/src/main/resources/manifest.json

The pipeline updates it on the fly with a call to (for example for fhir.r4.ukcore.stu3.currentbuild)

https://packages.simplifier.net/fhir.r4.ukcore.stu3.currentbuild

The version it returns for fhir.r4.ukcore.stu3.currentbuild is 0.0.7-sprint-7-review not 0.0.3-pre-release. So, removing the python dependency here implies that

https://github.com/NHSDigital/validation-service-fhir-r4/blob/main2/src/main/resources/manifest.json

Would need to be kept up to date with the latest version instead of setting it on the fly. I was asking whether updating it manually was more desirable or not. Apart from that, I think its a bit misleading having the manifest have a value in there that is wrong. Maybe instead of

  {
    "packageName": "fhir.r4.ukcore.stu3.currentbuild",
    "version": "0.0.3-pre-release"
  }

you'd have

  {
    "packageName": "fhir.r4.ukcore.stu3.currentbuild",
    "version": <PLEASE UPDATE>
  }

Pretty sure that would crash a local build and force whoever wanted to use it to update it. This would mean you could keep the update to the manifest to the latest, but a locally build would require an obvious update to the version. This would then also be useful to add to the readme. Whether that is done with python or not is another question, although regardless poetry is not needed, the only package required is requests which can be installed with pip.

Trying adding cache for maven to see if it improves workflow time
@declankieran
Copy link
Copy Markdown
Contributor Author

Would be worth adding the maven cache to this as well #25

Appears to have just added 9 seconds to the workflow time, theres only one maven build here so makes sense....

This

image

Compare to previous run

image

Going to remove it since it adds nothing

Remove unnecessary cache
@ryma2fhir
Copy link
Copy Markdown
Contributor

Would be worth adding the maven cache to this as well #25

Appears to have just added 9 seconds to the workflow time, theres only one maven build here so makes sense....

This

image

Compare to previous run

image

Going to remove it since it adds nothing

That's not what I'm seeing. The first run creates the cache, runs afterwards uses that cache.
1st run
image
2nd run
image
so 2m 35s v 1m37s takes approx 1 min off from the build

@ryma2fhir
Copy link
Copy Markdown
Contributor

It is used in the validation repo, but has been updated manually by KM as and when we have a new UKCore / NHSE package. Whether we would want manual control over which package we would like to validate against is another thing. I think we would, but we could always add a ticket to automate it and decide another day.

This is the manifest that is in main2

https://github.com/NHSDigital/validation-service-fhir-r4/blob/main2/src/main/resources/manifest.json

The pipeline updates it on the fly with a call to (for example for fhir.r4.ukcore.stu3.currentbuild)

https://packages.simplifier.net/fhir.r4.ukcore.stu3.currentbuild

The version it returns for fhir.r4.ukcore.stu3.currentbuild is 0.0.7-sprint-7-review not 0.0.3-pre-release. So, removing the python dependency here implies that

https://github.com/NHSDigital/validation-service-fhir-r4/blob/main2/src/main/resources/manifest.json

Would need to be kept up to date with the latest version instead of setting it on the fly. I was asking whether updating it manually was more desirable or not. Apart from that, I think its a bit misleading having the manifest have a value in there that is wrong. Maybe instead of

  {
    "packageName": "fhir.r4.ukcore.stu3.currentbuild",
    "version": "0.0.3-pre-release"
  }

you'd have

  {
    "packageName": "fhir.r4.ukcore.stu3.currentbuild",
    "version": <PLEASE UPDATE>
  }

Pretty sure that would crash a local build and force whoever wanted to use it to update it. This would mean you could keep the update to the manifest to the latest, but a locally build would require an obvious update to the version. This would then also be useful to add to the readme. Whether that is done with python or not is another question, although regardless poetry is not needed, the only package required is requests which can be installed with pip.

@KevinMayfield Is there any reason we are updating EPS's manifest.json? Otherwise we will remove this piece of code

@declankieran
Copy link
Copy Markdown
Contributor Author

That's not what I'm seeing. The first run creates the cache, runs afterwards uses that cache.

Fair enough, I've reran the action and it didn't redownload the packages. I wasn't thinking it shared the packages across runners, nice! I'll add it back in.

Adding cache back in, shared between runners so stops a redownload on maven build
@ryma2fhir
Copy link
Copy Markdown
Contributor

After some thought I think the python script to get the latest packages from simplifier would be great. I can't see a detriment to this, but it would save a manual step which can easily be missed. @declankieran-nhsd Could you do this? Either in this PR or another, whichever makes more sense.

Other than that the following can be removed as they are already install in ubuntu-latest:

  • Install Node version 16
  • Set up JDK 11

Comment thread .github/workflows/testingbranch.yml Outdated
Comment thread .github/workflows/testingbranch.yml Outdated
Copy link
Copy Markdown
Contributor

@APageNHS APageNHS left a comment

Choose a reason for hiding this comment

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

This all seems sensible and to improve the speed

@ryma2fhir ryma2fhir merged commit 9016ad8 into main Nov 10, 2023
@ryma2fhir ryma2fhir deleted the refactor-testingbranch-workflow branch November 10, 2023 09:39
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