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

fix: working sample 04 #92

Conversation

matthiasdg
Copy link
Contributor

What this PR changes/adds

fixes #86; commands from README now work.

Why it does that

basically looked at transfer sample 01 and applied all dependencies/variables from there

Further notes

first experience with edc connector, so not sure if everything now behaves as it should. transfer happens + jaeger + prometheus work

Linked Issue(s)

Closes #86

Checklist

  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • documented code?
  • assigned appropriate label?
  • formatted title correctly? (take a look at the CONTRIBUTING for details) -> ENDS IN 404

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

'We are always happy to welcome new contributors ❤️ To make things easier for everyone, please - make sure to follow our contribution guidelines, - check if you have already signed the ECA, and - relate this pull request to an existing issue or discussion.'

@ndr-brt ndr-brt self-requested a review July 28, 2023 06:58
@ndr-brt ndr-brt added the bug Something isn't working label Jul 28, 2023
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution, since you are here, could you take care on adding an integration test in the same way other samples are tested? so we'd avoid regressions in the future

@matthiasdg
Copy link
Contributor Author

@ndr-brt to have an integration test that covers the differences with the transfer-01 sample requires including the docker-compose in the test.

As an experiment I did that, using the testcontainers framework. Note that running this test requires the downloaded opentelemetry-javaagent as per the README + docker-compose on the local machine. (Without docker-compose is also an option, but that seems to require a very large heap space, did not get there).

Other than that, the test is almost identical to the transfer-01 test. I did not run assertTransferProcessStatusConsumerSide, since there is actually also an error like No checker found for process 2bfb0f52-15f4-4028-8337-ccd1c4144de1. The process will not advance to the COMPLETED state.. Not sure if this is related to eclipse-edc/Connector#3334 or due to some missing configuration

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

test is broke because it expects to find the agent jar, but it doesn't

@matthiasdg
Copy link
Contributor Author

Yes, I put that in the comment. If you want to have a test like this and don't want to check in this file (README already mentioned the manual download), maybe can be downloaded from the github action. Also not sure whether this will run on a github actions runner.
The waitingFor can probably also use some improvement

@ndr-brt
Copy link
Member

ndr-brt commented Aug 9, 2023

@matthiasdg We could have it downloaded by gradle as it has been implemented by tractusx

@github-actions
Copy link

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale Open for x days with no activity label Aug 17, 2023
@Jens011203
Copy link

Hello, the issue is still not solved. If you have time, it would be nice if you could commit the code to the main branch.😀

@github-actions github-actions bot removed the stale Open for x days with no activity label Aug 23, 2023
@github-actions
Copy link

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale Open for x days with no activity label Aug 30, 2023
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

This pull request was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this Sep 6, 2023
@ndr-brt
Copy link
Member

ndr-brt commented Sep 6, 2023

@matthiasdg we were near to the solution, could you take care of fixing the CI? I will merge then

@ndr-brt ndr-brt reopened this Sep 6, 2023
@github-actions github-actions bot removed the stale Open for x days with no activity label Sep 7, 2023
@github-actions
Copy link

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale Open for x days with no activity label Sep 15, 2023
@github-actions
Copy link

This pull request was closed because it has been inactive for 7 days since being marked as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale Open for x days with no activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer Sample 04 terminating immediately after startup
3 participants