Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import Licence versions #1195

Merged
merged 96 commits into from
Aug 27, 2024
Merged

Import Licence versions #1195

merged 96 commits into from
Aug 27, 2024

Conversation

jonathangoulding
Copy link
Collaborator

@jonathangoulding jonathangoulding commented Jul 18, 2024

https://eaflood.atlassian.net/browse/WATER-4575

We need to replace the import service logic to import a licence from NALD.

We will leave the import service running to add the NALD data in the import tables, but we will slowly replace the import-licence functionality and move this into the system repo.

This change will add the licence versions to the import logic.

The licence versions rely on the created / existing licence id.

This work needs to be done first to allow us to remove the licence and licence versions from the old import logic.

We need to replace the import service logic to import a licence from NALD.

We will leave the import service running to add the NALD data in the import tables, but we will slowly replace the import-licence functionality and move this into the system repo.

https://eaflood.atlassian.net/browse/WATER-4575

This change will add the licence versions to the import logic.

The licence versions rely on the created / existing licence id.

This work needs to be done first to allow us to remove the licence and licence versions from the old import logic.
app/validators/import/licence-versions.validator.js Outdated Show resolved Hide resolved
app/validators/import/licence.validator.js Outdated Show resolved Hide resolved
app/validators/import/licence.validator.js Outdated Show resolved Hide resolved
test/services/import/legacy-import/licence.mapper.test.js Outdated Show resolved Hide resolved
test/services/import/legacy-licence.service.test.js Outdated Show resolved Hide resolved
Cruikshanks added a commit that referenced this pull request Aug 19, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to replace the legacy licence import in readiness for ReSP

In [Import Licence](#1191) we added our first steps of migrating the legacy licence import from [water-abstraction-import](https://github.com/DEFRA/water-abstraction.import). Because this is still in development, we added some debug logging to check licence's the import service was pinging, and what data we were reading from NALD.

What we hadn't realised is that `console` calls, even `console.debug()` don't go through any kind of determination in Node.js. The calls to, for example, `console.info()`, `console.debug()`, or `console.error()` just determine whether Node outputs them to **stdout** or **stderr**.

So, they are _always_ output. If the logging was done through our [Hapi-pino plugin](https://github.com/hapijs/hapi-pino) then we can control the log levels. But we have no such control in Node.

We have got [a change in the works](#1195) where the debug calls are removed. But that is so large it is taking time to be processed. In the meantime, this debug output is starting to cause our non-prod environments to crash because they are filling up the limited disk space.

This quick change gets them out now.
Cruikshanks added a commit that referenced this pull request Aug 19, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to replace the legacy licence import in readiness for ReSP

In [Import Licence](#1191), we added our first steps of migrating the legacy licence import from [water-abstraction-import](https://github.com/DEFRA/water-abstraction.import). Because this is still in development, we added some debug logging to check the licence the import service was pinging and what data we were reading from NALD.

What we hadn't realised is that `console` calls, even `console.debug()`, don't go through any kind of determination in Node.js. The calls to, for example, `console.info()`, `console.debug()`, or `console.error()` just determine whether Node outputs them to **stdout** or **stderr**.

So, they are _always_ output. If the logging was done through our [Hapi-pino plugin](https://github.com/hapijs/hapi-pino) then we could control the log levels. But we have no such control in Node.

We have got [a change in the works](#1195) where the debug calls are removed. But that is so large it is taking time to be processed. In the meantime, this debug output is starting to cause our non-prod environments to crash because they are filling up the limited disk space.

This quick change gets them out now.
jonathangoulding and others added 2 commits August 20, 2024 10:10
Cruikshanks added a commit that referenced this pull request Aug 20, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to replace the legacy licence import in readiness for ReSP

In [Import Licence](#1191), we added our first steps of migrating the legacy licence import from [water-abstraction-import](https://github.com/DEFRA/water-abstraction.import).

At the same time we added some debug logging only to realise later [they were blowing up the logs](#1266)!

That's all sorted now, but we are still seeing a lot of traffic nightly appearing in the logs. It is to be expected because  [water-abstraction-import](https://github.com/DEFRA/water-abstraction-import) is pinging it for every licence it is importing (which is almost 80K! 😱)

We will be doing some form of logging when the [next iteration of the import](#1195) is approved and merged.

But till then we don't need to see that the endpoint was hit 80K times each night.

This change adds the route to our `HapiPinoIgnoreRequestService` which is used to control which routes are ignored in the logs.
Cruikshanks added a commit that referenced this pull request Aug 21, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to replace the legacy licence import in readiness for ReSP

In [Import Licence](#1191), we added our first steps of migrating the legacy licence import from [water-abstraction-import](https://github.com/DEFRA/water-abstraction.import).

At the same time, we added some debug logging only to realise later [that they were blowing up the logs](#1266)!

That's all sorted now, but we are still seeing a lot of traffic nightly appearing in the logs. It is to be expected because  [water-abstraction-import](https://github.com/DEFRA/water-abstraction-import) is pinging it for every licence it is importing (which is almost 80K! 😱)

We will log some information when the [next iteration of the import](#1195) is approved and merged.

But till then, we don't need to see that the endpoint was hit 80K times each night.

This change adds the route to our `HapiPinoIgnoreRequestService`, which controls which routes are ignored in the logs.
…icence-versions

# Conflicts:
#	app/controllers/import.controller.js
#	app/presenters/import/legacy/licence.presenter.js
#	app/services/import/legacy-import/fetch-licence-versions.service.js
#	app/services/import/legacy-import/fetch-licence.service.js
#	app/services/import/legacy-licence.service.js
#	app/services/import/licence-validator.service.js
#	app/validators/custom/date.validators.js
#	app/validators/import/licence.validator.js
Cruikshanks added a commit that referenced this pull request Aug 23, 2024
https://eaflood.atlassian.net/browse/WATER-4575

In this instance, we felt it was more constructive to provide the feedback for the original [Import Licence versions](#1195) proposed PR as a series of commits from where the code has been left.

This can be used to guide suggested changes, hopefully with better context than just PR comments.
Cruikshanks added a commit that referenced this pull request Aug 24, 2024
https://eaflood.atlassian.net/browse/WATER-4575

In this instance, we felt it was more constructive to provide the feedback for the original [Import Licence versions](#1195) proposed PR as a series of commits from where the code has been left.

This can be used to guide suggested changes, hopefully with better context than just PR comments.
https://eaflood.atlassian.net/browse/WATER-4575

In this instance, we felt it was more constructive to provide the feedback for the original [Import Licence versions](#1195) proposed PR as a series of commits from where the code has been left.

This can be used to guide suggested changes, hopefully with better context than just PR comments.

---

As I went through making these changes, I realised I was aiming for three outcomes;

- simplify the work of the mappers by moving casting and some data generation into the fetch service queries
- separate as much as possible the process for importing each entity (licence, licence version etc)
- persist as one using a DB transaction to avoid risk of partial licences

The commits themselves go into more details but very happy and glad to walk through the changes I'm suggesting.
@jonathangoulding jonathangoulding added the enhancement New feature or request label Aug 27, 2024
Cruikshanks added a commit to DEFRA/water-abstraction-import that referenced this pull request Aug 27, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to migrate the legacy licence import job

In [Import Licence versions](DEFRA/water-abstraction-system#1195) following some feedback we've decided to amend the endpoint that this repo will hit in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system) to trigger the import of a licence from NALD.

This change updates the URL to `/import/licence/legacy`.
Cruikshanks added a commit to DEFRA/water-abstraction-import that referenced this pull request Aug 27, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to migrate the legacy licence import job

In [Import Licence versions](DEFRA/water-abstraction-system#1195), following some feedback, we've decided to amend the endpoint that this repo will hit in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system) to trigger the import of a licence from NALD.

This change updates the URL to `/import/licence/legacy`.
@jonathangoulding jonathangoulding merged commit f9b36c3 into main Aug 27, 2024
6 checks passed
@jonathangoulding jonathangoulding deleted the import-licence-versions branch August 27, 2024 10:33
jonathangoulding added a commit that referenced this pull request Aug 27, 2024
https://eaflood.atlassian.net/browse/WATER-4649

We need to replace the import service logic to import a licence from NALD.

We will leave the import service running to add the NALD data in the import tables, but we will slowly replace the import-licence functionality and move this into the system repo.

This change will add the licence versions purpose conditionsto the import logic.

Previous work work add licence version and licence version purpose - #1195
jonathangoulding added a commit that referenced this pull request Aug 27, 2024
https://eaflood.atlassian.net/browse/WATER-4649

We need to replace the import service logic to import a licence from NALD.

We will leave the import service running to add the NALD data in the import tables, but we will slowly replace the import-licence functionality and move this into the system repo.

This change will add the licence versions purpose conditionsto the import logic.

Previous work work add licence version and licence version purpose - #1195
jonathangoulding added a commit that referenced this pull request Aug 30, 2024
* Import Licence version purpose conditions

https://eaflood.atlassian.net/browse/WATER-4649

We need to replace the import service logic to import a licence from NALD.

We will leave the import service running to add the NALD data in the import tables, but we will slowly replace the import-licence functionality and move this into the system repo.

This change will add the licence versions purpose conditionsto the import logic.

Previous work work add licence version and licence version purpose - #1195

---------

Co-authored-by: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Cruikshanks added a commit that referenced this pull request Sep 6, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to replace the legacy licence import in readiness for ReSP

Argh! We've done it again! We made a change to [Ignore new /import/licence endpoint in logs](#1275) so the logs became silent again instead of outputting 80K requests!

But then in [Import Licence versions](#1195) we tweaked the route, and guess what we forgot to do! 😱🤦

This change updates the `HapiPinoIgnoreRequest` with the updated route.
Cruikshanks added a commit that referenced this pull request Sep 6, 2024
https://eaflood.atlassian.net/browse/WATER-4575

> Part of the work to replace the legacy licence import in readiness for ReSP

Argh! We've done it again! We made a change to [Ignore new /import/licence endpoint in logs](#1275), so the logs became silent again instead of outputting 80K requests!

But then in [Import Licence versions](#1195) we tweaked the route, and guess what we forgot to do! 😱🤦

This change updates the `HapiPinoIgnoreRequest` with the updated route.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants