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

PIN-5961: added delegation-items-archiver scaffold #1374

Open
wants to merge 5 commits into
base: feature/incaricato
Choose a base branch
from

Conversation

AsterITA
Copy link
Contributor

@AsterITA AsterITA commented Jan 15, 2025

[Closes PIN-5961]

Copy link
Collaborator

@ecamellini ecamellini left a comment

Choose a reason for hiding this comment

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

👌 two super minor comments, no strong opinion

? unsafeBrandId<CorrelationId>(delegationMsg.correlation_id)
: generateId<CorrelationId>();

const loggerInstance = logger({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move the logger setup in the processMessage main function in the index, so that we set up everything there, before routing from a message to its handler

Comment on lines 14 to 16
// import { getInteropBeClients } from "./clients/clientsProvider.js";

// const { agreementProcessClient, purposeProcessClient } = getInteropBeClients();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for the clients, let's set them up in the index and pass them as params to handleMessage

Copy link
Collaborator

@ecamellini ecamellini left a comment

Choose a reason for hiding this comment

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

I find the new name less meaningful: delegation archiver seems to be something that archives the delegations, but, instead, it reacts to delegation archive events and archives something else. See for reference eservice-descriptors-archiver: a consumer that reacts to agreements archive events and archives descriptors.

What about something like delegated-agreements-purposes-archiver or delegated-items-archiver or something similar?

@AsterITA
Copy link
Contributor Author

I find the new name less meaningful: delegation archiver seems to be something that archives the delegations, but, instead, it reacts to delegation archive events and archives something else. See for reference eservice-descriptors-archiver: a consumer that reacts to agreements archive events and archives descriptors.

What about something like delegated-agreements-purposes-archiver or delegated-items-archiver or something similar?

I agree with changing its name, how about delegation-items-archiver?

@ecamellini
Copy link
Collaborator

@AsterITA delegation-items-archiver sounds perfect 🤝

@AsterITA AsterITA changed the title feat: added delegation-revoke-archiver scaffold PIN-5961: added delegation-items-archiver scaffold Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants