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

TE-644: Fix review code to release first version #1

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

trungmaihova
Copy link
Collaborator

No description provided.

Copy link
Contributor

github-actions bot commented Aug 15, 2024

Test Results

10 tests  ±0   10 ✅ ±0   12s ⏱️ +3s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 2bb633c. ± Comparison against base commit 8985c98.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@ntqdinh-axonivy ntqdinh-axonivy left a comment

Choose a reason for hiding this comment

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

LGTM

@ntqdinh-axonivy ntqdinh-axonivy merged commit d892d7d into develop Aug 15, 2024
4 checks passed
Copy link
Member

@ivy-rew ivy-rew left a comment

Choose a reason for hiding this comment

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

@ntqdinh-axonivy it seems like this connector is not built upon the ivy REST stack; I think that would be very important. This is a responsibility of team Octopus throughout the review to verify the technical approach for integrating well into the existing Axonivy environment. So I miss a discussion on this 🤔

Let me point out some features we miss by using the azure-sdk:

  • no automatic logs on remote calls
  • no tracing support to identify failing or slow requests
  • limited configuration support to control the URIs or users
  • no support to test the connectability within the cockpit

We may can continue by using the azure-sdk, but the HTTP client it uses should be Ivy.rest/Jersey ... otherwise we loose all the power of the platform supporting this connector.

@ntqdinh-axonivy
Copy link
Collaborator

@ntqdinh-axonivy it seems like this connector is not built upon the ivy REST stack; I think that would be very important. This is a responsibility of team Octopus throughout the review to verify the technical approach for integrating well into the existing Axonivy environment. So I miss a discussion on this 🤔

Let me point out some features we miss by using the azure-sdk:

  • no automatic logs on remote calls
  • no tracing support to identify failing or slow requests
  • limited configuration support to control the URIs or users
  • no support to test the connectability within the cockpit

We may can continue by using the azure-sdk, but the HTTP client it uses should be Ivy.rest/Jersey ... otherwise we loose all the power of the platform supporting this connector.

@ivy-rew Thanks for your feedback.
We will release a new version soon to make it much more match with AxonIvy platform.

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.

3 participants