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

jakarta upgrade #687

Merged
merged 2 commits into from
Nov 16, 2024
Merged

jakarta upgrade #687

merged 2 commits into from
Nov 16, 2024

Conversation

smahieu87
Copy link
Contributor

Pull Request Description

Please include a summary of the change request or bug which is supposed to be fixed as part of this pull request

Type of Pull Request

  • New feature/Enhancement - non-breaking change which adds functionality
  • Bug fix
  • Breaking change (Require Major version change?)

Type of Change

  • OpenPeppol AS2/AS4 specification
  • OpenPeppol Spring/Fall release
  • Oxalis software change or enhancement
  • CEF change

Pull Request Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas. But did not added unnecessary annotation/comment say @author name etc
  • I have checked my code for variable and method name and corrected grammar/spelling mistakes if any
  • I have made corresponding changes to the documentation where needed
  • My changes generate no new/additional warnings
  • My change is not breaking or creating conflict with associated dependencies
  • I have performed a self-review of my own code
  • I ran mvn clean install before commit and all tests run successfully
  • I conducted basic QA to assure all features are working fine
  • My pull request generate no conflicts with master branch
  • I requested code review from other team members

@aaron-kumar aaron-kumar self-assigned this Oct 26, 2024
aaron-kumar
aaron-kumar reviewed Nov 2, 2024
View reviewed changes
Copy link
Member

@aaron-kumar aaron-kumar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@smahieu87 Please fix minor comments. Once done, PR will be approved.

Self Note: Once Pull request will be approved then QA testing will be required to analyze the impact of opentracing span wrt opentelemetry span etc.

oxalis-commons/src/main/java/network/oxalis/commons/mode/ModeProvider.java
import network.oxalis.api.lang.OxalisLoadingException;
import network.oxalis.commons.certvalidator.api.CrlFetcher;
import network.oxalis.pkix.ocsp.api.OcspFetcher;
Copy link
Member

Choose a reason for hiding this comment

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

Do Not unnecessarily rearrange code i.e. do Not remove and add same dependencies e.g.

import network.oxalis.commons.certvalidator.api.CrlFetcher;
import network.oxalis.pkix.ocsp.api.OcspFetcher;

oxalis-commons/src/main/java/network/oxalis/commons/mode/OxalisCrlFetcher.java
if (span.getSpan() != null)
basicHttpContext.setAttribute(Constants.PARENT_CONTEXT, span.getSpan().context());
if (span != null)
basicHttpContext.setAttribute("OxalisCrlFetcher.parentSpanContext", span.getSpanContext());
Copy link
Member

Choose a reason for hiding this comment

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

Better option is to replace below statement:
basicHttpContext.setAttribute("OxalisCrlFetcher.parentSpanContext", span.getSpanContext());
with
basicHttpContext.setAttribute(OxalisCrlFetcher.class.getName() + ".parentSpanContext", span.getSpanContext());

oxalis-commons/src/main/java/network/oxalis/commons/mode/OxalisOcspFetcher.java
if (span.getSpan() != null)
basicHttpContext.setAttribute(Constants.PARENT_CONTEXT, span.getSpan().context());
if (span != null)
basicHttpContext.setAttribute("OxalisOcspFetcher.parentSpanContext", span.getSpanContext());
Copy link
Member

Choose a reason for hiding this comment

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

Better option is to replace below statement:
basicHttpContext.setAttribute("OxalisOcspFetcher.parentSpanContext", span.getSpanContext());
with
basicHttpContext.setAttribute(OxalisOcspFetcher.class.getName() + ".parentSpanContext", span.getSpanContext());

oxalis-extension/oxalis-as2/src/main/java/network/oxalis/as2/outbound/As2MessageSender.java
try (CloseableHttpClient httpClient = httpClientProvider.get()) {
BasicHttpContext basicHttpContext = new BasicHttpContext();
basicHttpContext.setAttribute(Constants.PARENT_CONTEXT, span.context());
basicHttpContext.setAttribute("spanContext", span.getSpanContext());
Copy link
Member

Choose a reason for hiding this comment

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

Better option is to replace below statement:
basicHttpContext.setAttribute("spanContext", span.getSpanContext());
with
basicHttpContext.setAttribute(As2MessageSender.class.getName() + ".parentSpanContext", span.getSpanContext());

...-outbound/src/main/java/network/oxalis/outbound/transmission/DefaultTransmissionService.java
import network.oxalis.api.outbound.TransmissionResponse;
import network.oxalis.api.outbound.TransmissionService;
import network.oxalis.api.outbound.Transmitter;
import network.oxalis.api.tag.Tag;
Copy link
Member

Choose a reason for hiding this comment

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

Why unnecessary formatting (removing and adding import network.oxalis.api.tag.Tag) ?

...-outbound/src/main/java/network/oxalis/outbound/transmission/TransmissionRequestFactory.java
import io.opentracing.Tracer;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.Tracer;
import jakarta.inject.Inject;
Copy link
Member

Choose a reason for hiding this comment

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

Just replace, avoid unnecessary addition and removal

pom.xml Outdated
<httpclient.version>4.5.13</httpclient.version>
<commons-codec.version>1.15</commons-codec.version>
<jetty.version>10.0.16</jetty.version>
<jetty.version>11.0.24</jetty.version>
Copy link
Member

Choose a reason for hiding this comment

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

This is Not directly linked with PR.

@aaron-kumar aaron-kumar added this to the 7.x.x milestone Nov 15, 2024
@aaron-kumar
Copy link
Member

aaron-kumar commented Nov 16, 2024 •
edited
Loading

Merging pull request "as-is" as no action was taken. There will be build failure because of this pull request which I will fix with another commit.

@aaron-kumar aaron-kumar merged commit 56a3896 into OxalisCommunity:master Nov 16, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaron-kumar aaron-kumar aaron-kumar left review comments

Assignees

@aaron-kumar aaron-kumar

Labels
Enhancement
Projects
Oxalis Public Roadmap
Status: Q4 2024 - September - December
Milestone
7.x.x
Development

Successfully merging this pull request may close these issues.
2 participants
@smahieu87 @aaron-kumar