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

feat(CxMembership): Set Catena-X Membership in BPDM #1118

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tfjanjua
Copy link
Contributor

@tfjanjua tfjanjua commented Oct 25, 2024

Description

Set Catena-X Membership in BPDM.

Why

As Portal, we would like to maintain a list of all companies that were onboarded to the Catena-X network. This membership information is meant to be stored in the BPDM Pool. We therefore need to update the isCatenaXMember flag for a BPNL once the company has been onboarded.

Issue

Ref: #1111

Config PR: eclipse-tractusx/portal#472

BPDM PR: eclipse-tractusx/bpdm#1072

Checklist

  • I have followed the contributing guidelines
  • I have performed a self-review of my own code
  • I have successfully tested my changes locally
  • I have added tests that prove my changes work
  • I have checked that new and existing tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas

@tfjanjua tfjanjua self-assigned this Oct 25, 2024
@tfjanjua tfjanjua added the enhancement New feature or request label Oct 25, 2024
@tfjanjua tfjanjua force-pushed the chore/1111-Set-Catena-X-Membership-in-BPDM branch from 05892e0 to 508af80 Compare October 28, 2024 19:03
@tfjanjua tfjanjua marked this pull request as ready for review October 30, 2024 12:43
@tfjanjua tfjanjua requested review from ntruchsess and Phil91 October 30, 2024 12:43
@ntruchsess ntruchsess linked an issue Nov 20, 2024 that may be closed by this pull request
4 tasks
@ntruchsess ntruchsess self-assigned this Nov 20, 2024
@ntruchsess ntruchsess force-pushed the chore/1111-Set-Catena-X-Membership-in-BPDM branch from 8a0304f to c649d4d Compare November 20, 2024 14:44
Copy link
Contributor

@ntruchsess ntruchsess left a comment

Choose a reason for hiding this comment

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

@tfjanjua : I did fix a few findings and force-pushed to your repo after rebasing on latest main. Please pull he changes before proceeding. (And also have a look on what I changed to avoid such findings in the future).
PR currently cannot be merged as the configuration in portal-repo does not match. Also we need to come up with a better approach to handle multiple bpdm deployments without hardcoding the fact that this particular call is different into the service-method.


// Same User (which we have for BPDM service) can be used to call Business Partner Pool
// But BaseAddress of Business Partner Pool is different as its deployed on another server.
httpClient.BaseAddress = new Uri(_settings.BusinessPartnerPoolBaseAddress);
Copy link
Contributor

@ntruchsess ntruchsess Nov 20, 2024

Choose a reason for hiding this comment

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

(while technically working) this is not how a httpClientFactory (which is used in tokenService) should be used. The service-code should not have to deal with the baseAddress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntruchsess I totally agree to not assign value to BaseAddress param at library when its already managed in common token service but here we have a situation that we want to call this newly introduced method SetCxMembership from background process and not from the controller (because there we have a possibility to take user's token use same token to call Business Partner Pool api route) and same user (which has been used to call BPDM services) can be used to call Business Partner Pool service so, thats why I introduced another kind of baseAddress BusinessPartnerPoolBaseAddress within the same BPDM configs and override the value of BaseAddress of BPDM config before calling the tokenService so, I could use same user (which has been used to call BPDM services) and could get the token to call this Business Partner Pool service.

It would be nice if we could separate out BpnAccess service from BPDM library because I think then we can avoid assigning or overriding the value of BaseAddress because then we would have opportunity to have proper user (with baseAddress) to call Business Partner Pool services (no matter from controller or background process).

@tfjanjua
Copy link
Contributor Author

@tfjanjua : I did fix a few findings and force-pushed to your repo after rebasing on latest main. Please pull he changes before proceeding. (And also have a look on what I changed to avoid such findings in the future). PR currently cannot be merged as the configuration in portal-repo does not match. Also we need to come up with a better approach to handle multiple bpdm deployments without hardcoding the fact that this particular call is different into the service-method.

Thank you @ntruchsess , providing your feedback.

I've looked at your commit and found some formatting and addition of license related information (especially in migration files).

I'll have a look at configuration PR.

@tfjanjua
Copy link
Contributor Author

tfjanjua commented Dec 9, 2024

@tfjanjua : I did fix a few findings and force-pushed to your repo after rebasing on latest main. Please pull he changes before proceeding. (And also have a look on what I changed to avoid such findings in the future). PR currently cannot be merged as the configuration in portal-repo does not match. Also we need to come up with a better approach to handle multiple bpdm deployments without hardcoding the fact that this particular call is different into the service-method.

Thank you @ntruchsess , providing your feedback.

I've looked at your commit and found some formatting and addition of license related information (especially in migration files).

I'll have a look at configuration PR.

@ntruchsess , I have added my comment in the corresponding config PR: eclipse-tractusx/portal#472 (comment)

You may resolve the conversation if the provided explanations make sense to you as well.

Thanks

@tfjanjua tfjanjua requested a review from ntruchsess December 9, 2024 10:52
@evegufy evegufy changed the title chore(CxMembership): Set Catena-X Membership in BPDM feat(CxMembership): Set Catena-X Membership in BPDM Dec 13, 2024
@evegufy
Copy link
Contributor

evegufy commented Dec 13, 2024

@tfjanjua could you please resolve the conflict?
@ntruchsess @Phil91 could you please review?

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
Status: IN PROGRESS
Development

Successfully merging this pull request may close these issues.

Process Worker | Set Catena-X Membership in BPDM
4 participants