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

Preregister Kafka Health #1023

Merged
merged 3 commits into from May 27, 2022
Merged

Preregister Kafka Health #1023

merged 3 commits into from May 27, 2022

Conversation

PeterSP
Copy link

@PeterSP PeterSP commented May 26, 2022

πŸ“š Purpose

Register Kafka Health-Check components immediately, prior to successful connection.

πŸ‘Œ Resolves:

  • πŸ› PLAT-530 patches bug

πŸ“¦ Impacts:

[long list TBD]

βœ… PR Checklist

  • Add changeset

@changeset-bot
Copy link

changeset-bot bot commented May 26, 2022

πŸ¦‹ Changeset detected

Latest commit: 80be4a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 50 packages
Name Type
@container-images/mds-agency Patch
@container-images/mds-attachment-service Patch
@container-images/mds-audit-api Patch
@container-images/mds-audit-service Patch
@container-images/mds-collector-api Patch
@container-images/mds-collector-service Patch
@container-images/mds-compliance-api Patch
@container-images/mds-compliance-service Patch
@container-images/mds-geography-api Patch
@container-images/mds-geography-author-api Patch
@container-images/mds-geography-service Patch
@container-images/mds-ingest-service Patch
@container-images/mds-jurisdiction-api Patch
@container-images/mds-jurisdiction-service Patch
@container-images/mds-policy Patch
@container-images/mds-policy-author-api Patch
@container-images/mds-policy-service Patch
@container-images/mds-provider-service Patch
@container-images/mds-transaction-api Patch
@container-images/mds-transaction-service Patch
@mds-core/mds-agency Patch
@mds-core/mds-api-helpers Patch
@mds-core/mds-attachment-service Patch
@mds-core/mds-audit-api Patch
@mds-core/mds-audit-service Patch
@mds-core/mds-collector-api Patch
@mds-core/mds-collector-service Patch
@mds-core/mds-compliance-api Patch
@mds-core/mds-compliance-engine Patch
@mds-core/mds-compliance-service Patch
@mds-core/mds-config-api Patch
@mds-core/mds-db Patch
@mds-core/mds-geography-api Patch
@mds-core/mds-geography-author-api Patch
@mds-core/mds-geography-service Patch
@mds-core/mds-ingest-service Patch
@mds-core/mds-jurisdiction-api Patch
@mds-core/mds-jurisdiction-service Patch
@mds-core/mds-policy Patch
@mds-core/mds-policy-author-api Patch
@mds-core/mds-policy-service Patch
@mds-core/mds-provider-service Patch
@mds-core/mds-repository Patch
@mds-core/mds-test-data Patch
@mds-core/mds-transaction-api Patch
@mds-core/mds-transaction-service Patch
@mds-core/mds-compliance-batch-processor Patch
@mds-core/mds-stream Patch
@container-images/mds-config-api Patch
@mds-core/mds-stream-processor Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@PeterSP PeterSP requested a review from avatarneil May 26, 2022 21:26
@PeterSP PeterSP self-assigned this May 26, 2022
@PeterSP PeterSP force-pushed the PLAT-530-preregister-kafka-health branch from 62e2394 to c87ac7e Compare May 26, 2022 21:57
@PeterSP PeterSP marked this pull request as ready for review May 26, 2022 21:58
@@ -18,7 +18,7 @@
"images": "pnpm run:concurrent image",
"preinstall": "npx check-node-version --package && npx only-allow pnpm && pnpm i -g pnpm@6.32.11",
"lint": "pnpm lint:audit && pnpm lint:prettier '**/package.json' && pnpm lint:markdown && pnpm lint:eslint '**/*.ts'",
"lint:audit": "pnpm-ci audit --strict -i GHSA-fwr7-v2mv-hh25",
"lint:audit": "pnpm-ci audit --strict -i GHSA-fwr7-v2mv-hh25 GHSA-wm7h-9275-46v2",
Copy link
Author

Choose a reason for hiding this comment

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

We can undo this once expressjs/multer#1097 is merged.

@PeterSP PeterSP requested a review from avatarneil May 26, 2022 23:46
Copy link

@avatarneil avatarneil left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@mdurling mdurling left a comment

Choose a reason for hiding this comment

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

Minor nit re delete usage, but LGTM.

@@ -121,6 +126,7 @@ const createStreamConsumer = async (
await Promise.all(asArray(topics).map(topic => consumer.subscribe({ topic, fromBeginning })))

registerLivelinessHandling(consumer, healthStatus)
delete healthStatus.components[preConnectComponentId]

Choose a reason for hiding this comment

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

I don't love using delete (I generally try to avoid), but the alternatives are somewhat cumbersome.

Copy link
Author

Choose a reason for hiding this comment

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

It didn't seem worth doing another destructure and assignment when the delete would suffice

@PeterSP PeterSP merged commit 2927d78 into develop May 27, 2022
@PeterSP PeterSP deleted the PLAT-530-preregister-kafka-health branch May 27, 2022 15:43
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.

None yet

4 participants