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

Feature/179 scaffold #1

Merged
merged 18 commits into from
Jun 2, 2022
Merged

Feature/179 scaffold #1

merged 18 commits into from
Jun 2, 2022

Conversation

algattik
Copy link
Contributor

@algattik algattik commented Jun 1, 2022

Creates scaffold Registry service web service as an EDC runtime with simple health endpoint.

CI pipeline builds the registration service and runs it in docker. A system test runs an HTTP request as a client to validate the build.

Closes https://github.com/agera-edc/MinimumViableDataspace/issues/179

On naming, we use org.eclipse.dataspaceconnector.registration since the repo will live in the https://github.com/eclipse-dataspaceconnector organization.

@github-actions
Copy link

github-actions bot commented Jun 2, 2022

Unit Test Results

1 files  1 suites   0s ⏱️
1 tests 1 ✔️ 0 💤 0

Results for commit 46c362d.

♻️ This comment has been updated with latest results.

@algattik algattik marked this pull request as ready for review June 2, 2022 06:00
@cpeeyush cpeeyush self-requested a review June 2, 2022 07:52
@Izzzu Izzzu self-requested a review June 2, 2022 08:02
@ouphi ouphi self-requested a review June 2, 2022 08:46
}
configurations {
all {
exclude(group = "com.fasterxml.jackson.jaxrs", module = "jackson-jaxrs-json-provider")
Copy link

Choose a reason for hiding this comment

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

Do you know why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but that's how it is in EDC and removing it gives a missing dependency error

docs/developer/openapi.md Outdated Show resolved Hide resolved
@Produces({"application/json"})
@Consumes({"application/json"})
@Path("/health")
public class HealthApiController {
Copy link

Choose a reason for hiding this comment

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

Did you consider using EDC's ObservabilityApiController?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I did want to add a custom controller to show how it's done, but this can be a follow-up story after we have more controllers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


@Target({ElementType.TYPE, ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@EnabledIfEnvironmentVariable(named = "INTEGRATION_TEST", matches = "true")
Copy link

Choose a reason for hiding this comment

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

  • we can add a follow up story to add doc about details on how to run integration test.
  • And also, a system test README explaining that project needs to be build first to generate executable jar ./build/libs/app.jar and then only docker-compose will work locally.

Both of these items can be done in a follow-up story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added system test README.md

@algattik algattik requested review from cpeeyush and removed request for marcgs June 2, 2022 10:11
Copy link

@cpeeyush cpeeyush left a comment

Choose a reason for hiding this comment

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

Nice work!


@IntegrationTest
public class HealthApiClientTest {
static final String API_URL = "http://localhost:8181/api";
Copy link

Choose a reason for hiding this comment

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

very nit: maybe we can read it from environment variables and keep this one as default. or we can do it later as well when we extend the test to verify health endpoint of a deployed one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I first had a version with env var, but then I removed it along the principle to only implement things once we have a clear need for them :)

@algattik algattik merged commit d9f81a3 into main Jun 2, 2022
@algattik algattik deleted the feature/179-scaffold branch June 2, 2022 11:55
cpeeyush pushed a commit that referenced this pull request Jun 15, 2022
cpeeyush pushed a commit that referenced this pull request Jun 15, 2022
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.

4 participants