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.

Sign up for GitHub

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

Jump to bottom

Use createJsGraphBuilderFactoryFromConfig function from FrameworkUtils #147

Merged
merged 6 commits into from
Nov 28, 2023
Merged

Use createJsGraphBuilderFactoryFromConfig function from FrameworkUtils #147

merged 6 commits into from
Nov 28, 2023

Conversation

RushanNanayakkara
Copy link
Contributor

Purpose

Use createJsGraphBuilderFactoryFromConfig function from FrameworkUtils as using the function from internal package causes class not found exception since internal package is not exported.

Related Issue

wso2/product-is#17132

nilasini
nilasini reviewed Nov 21, 2023
View reviewed changes
.../wso2/carbon/identity/conditional/auth/functions/notification/SendEmailFunctionImplTest.java
@@ -58,6 +59,7 @@ protected void setUp() throws Exception {

super.setUp();

CarbonConstants.ENABLE_LEGACY_AUTHZ_RUNTIME = true;
Copy link

Choose a reason for hiding this comment

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

what is the usecase behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in the pre test set ups. However there is no default value provided for this in CarbonConstants. Therefore this gives a NullPointerException when tests are run. Therefore the value was initialised here before tests run.

nilasini
nilasini reviewed Nov 21, 2023
View reviewed changes
components/org.wso2.carbon.identity.conditional.auth.functions.user/pom.xml
@@ -104,6 +104,19 @@
<artifactId>nashorn-core</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.wso2.orbit.com.nimbusds</groupId>
<artifactId>nimbus-jose-jwt</artifactId>
Copy link

Choose a reason for hiding this comment

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

why are we adding these new dependencies/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were build issues due to this dependency not being present. There are multiple usages of this in the codebase however the dependency was missing. Therefore there were compilation issues.

@RushanNanayakkara RushanNanayakkara merged commit bec3224 into wso2-extensions:master Nov 28, 2023
1 check passed
@nilasini
Copy link

We have to trigger the PR builder before merging the PR
@RushanNanayakkara

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/7014862057

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/7014862057
Status: failure

@RushanNanayakkara
Copy link
Contributor Author

We have to trigger the PR builder before merging the PR @RushanNanayakkara

My bad. Will trigger a pr build for the version bump to test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShanChathusanda93 ShanChathusanda93 ShanChathusanda93 left review comments

@nilasini nilasini nilasini approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RushanNanayakkara @nilasini @jenkins-is-staging @ShanChathusanda93