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

feat: Setup deployment microservice #54

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gbanu
Copy link

@gbanu gbanu commented Nov 25, 2024

No description provided.

@gbanu gbanu requested a review from a team as a code owner November 25, 2024 00:28
@gbanu gbanu added the server label Nov 25, 2024
@gbanu gbanu changed the title Server: setup deployment microservice feat: setup deployment microservice Nov 25, 2024
@gbanu gbanu changed the title feat: setup deployment microservice feat: Setup deployment microservice Nov 25, 2024
Copy link
Contributor

@thielpa thielpa left a comment

Choose a reason for hiding this comment

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

LGTM, good work in my opinion 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

proposal: Seems like there are different spacings used in this file. I would propose to use a linter e.g. checkstyle also for the server part (of course in a separate issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

discussion: can we share some of them in a global .gitignore in the server folder for all microservices? Not sure though, but I do not like the code duplication

public record EnvironmentInfoDTO(
@NonNull Long id,
@NonNull String name,
@NonNull String description,
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Shouldn't the description be nullable (or empty string)?

@NonNull Status status,
@NonNull Long createdByUserId,
OffsetDateTime createdAt,
List<InstalledAppDTO> installedApps,
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Should installedApps be nullable?

environment.getInstalledApps()
.stream()
.map(InstalledAppDTO::toInstalledAppDTO)
.sorted()
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is this sorting by name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants