tools versions & cicd publish-stage & codespace devcontainer #105
Conversation
RossBugginsNHS
changed the title
| secrets: | ||
| APP_PEM_FILE: ${{ secrets.APP_PEM_FILE }} | ||
| APP_CLIENT_ID: ${{ secrets.APP_CLIENT_ID }} | ||
| publish-stage: # Recommended maximum execution time is 10 minutes |
There was a problem hiding this comment.
This might be better moved to pr_closed? There it will only run on main and after the acceptance tests have run.
There was a problem hiding this comment.
reason I've kept it here, is so i can push direct to main on a fork, and have this go through. otherwise on a fork when testing CICD to GHP, you would have to do your own pr to yourself.
There was a problem hiding this comment.
I don't think we should be making compromises to support forks.
There was a problem hiding this comment.
any suggestion on how to support forks?
There was a problem hiding this comment.
For this isolated example you can amend the workflow on the forked repo.
It's not supporting forks, no. If you want full fork support I think it would be best to move it out to its own repo.
There was a problem hiding this comment.
Think this is a wider conversation to fully move the publish stage out of here. Probably need to look at how our repository template (https://github.com/NHSDigital/nhs-notify-repository-template/tree/main/.github/workflows) is different from the NHSE template (https://github.com/nhs-england-tools/repository-template/tree/main/.github/workflows), in that we don't have the separate publish workflow, and that to get around that for this to be able to publish its just in https://github.com/NHSDigital/nhs-notify-digital-letters/blob/main/.github/workflows/cicd-1-pull-request.yaml instead.
There was a problem hiding this comment.
Would like to suggest that removing this at present is a breaking changes for a fork, so to leave it in. Then will look separately at bringing in the workflow "2 publish" and look at fully moving the publish stage out of this workflow?
There was a problem hiding this comment.
I'd prefer to leave this as-is while we decide on the best place for it.
There was a problem hiding this comment.
I'm not a fan of all these .tool-versions files. Do these packages need node 24?
There was a problem hiding this comment.
Unsure if they will play ball with 22. I do think this is the point of tools versions though so can do monorepo support for tools versioning?
There was a problem hiding this comment.
I'm not sure it works well with npm workspaces and make where most commands are ran from the root directory. If you are keeping them, I presume you don't need all those tools i.e. terraform stuff?
There was a problem hiding this comment.
most of the make uses make -C so it runs child ones from their directory. Have gone through and removed all redundant lines. Was a good spot though as the template doesn't proactively install child tools versions - something that we've missed as there is an existing tool-versions nested in there. Have done a separate pr to backport changes - see change to init.mk
RossBugginsNHS
changed the title
tdroza-nhs
left a comment
tdroza-nhs
left a comment
There was a problem hiding this comment.
Spoke with Ross about Ian's suggestions and happy to leave the changes as they are
…s files. Need to backport to nhse template
…ired, build fails with node 22
…fig is still running, otherwise new terminal opening gives the impression config has finished.
There was a problem hiding this comment.
most of the make uses make -C so it runs child ones from their directory. Have gone through and removed all redundant lines. Was a good spot though as the template doesn't proactively install child tools versions - something that we've missed as there is an existing tool-versions nested in there. Have done a separate pr to backport changes - see change to init.mk
| secrets: | ||
| APP_PEM_FILE: ${{ secrets.APP_PEM_FILE }} | ||
| APP_CLIENT_ID: ${{ secrets.APP_CLIENT_ID }} | ||
| publish-stage: # Recommended maximum execution time is 10 minutes |
There was a problem hiding this comment.
Think this is a wider conversation to fully move the publish stage out of here. Probably need to look at how our repository template (https://github.com/NHSDigital/nhs-notify-repository-template/tree/main/.github/workflows) is different from the NHSE template (https://github.com/nhs-england-tools/repository-template/tree/main/.github/workflows), in that we don't have the separate publish workflow, and that to get around that for this to be able to publish its just in https://github.com/NHSDigital/nhs-notify-digital-letters/blob/main/.github/workflows/cicd-1-pull-request.yaml instead.
Have put some more thought into the above and some additional changes in place |
Ian-Hodges
left a comment
Ian-Hodges
left a comment
There was a problem hiding this comment.
This PR is by-passing all the checks. There's no assurance that this PR won't break main when merged.
See #111 for PR from branch instead of fork - checks now running |
3 participants
CI CID publish
Currently the
publish-stageinci-cd-1-pull-request.yamlhasneeds: [metadata, acceptance-stage], this changes that toneeds: [metadata, build-stage]the
publish-stagehas a restriction ofif: (github.event_name == 'push' && github.ref == 'refs/heads/main')ensuringpublish-stageonly runs when commited to mainPossibly look next to reintroduce the separate publish workflow stage (as per https://github.com/nhs-england-tools/repository-template/blob/main/.github/workflows/cicd-2-publish.yaml)
Tools
update root tool versions file for node to be set to the latest lambda supported version. Sub directories have specific requirements.
Update init.mk to recursively load tool-versions files. Discussion on this at (nhs-england-tools/repository-template#195)
Readme
Added to src folder
Codespaces
Added dev container referencing
ghcr.io/nhsdigital/nhs-notify-devcontainer-loaded-codespaces:1.0.19to allow for codespace developmentRemove the auto loading of a new terminal, so it does not hide initial config in a dev container.
##Event Catalog
Bump to latest version