-
Notifications
You must be signed in to change notification settings - Fork 1
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: Locking and Deployment information changes #276
base: main
Are you sure you want to change the base?
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
Just for your info: You'll probably need to create a migration file for the entity changes and merge main into the feat branch. If you need any assistance just hit me up :) |
(a, b) -> { | ||
OffsetDateTime timeA = a.timestamp(); | ||
OffsetDateTime timeB = b.timestamp(); | ||
if (timeA == null && timeB == null) return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck> reported by reviewdog 🐶
'if' construct must use '{}'s.
OffsetDateTime timeA = a.timestamp(); | ||
OffsetDateTime timeB = b.timestamp(); | ||
if (timeA == null && timeB == null) return 0; | ||
if (timeA == null) return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck> reported by reviewdog 🐶
'if' construct must use '{}'s.
OffsetDateTime timeB = b.timestamp(); | ||
if (timeA == null && timeB == null) return 0; | ||
if (timeA == null) return 1; | ||
if (timeB == null) return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck> reported by reviewdog 🐶
'if' construct must use '{}'s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a review session with @TurkerKoc and below points will be changed 👍🏻
Also in an another PR we need to refactor this re-deploy logic where we have this hardcoded 20 minutes for any missing webhook events, we need to think about this case and how it effects our system.
client/src/app/components/environments/deployment-state-tag/deployment-state-tag.component.html
Outdated
Show resolved
Hide resolved
<i-tabler name="user" class="w-6 h-6 text-gray-600" /> | ||
} | ||
<span> | ||
{{ environment.latestDeployment.user?.name }} deployed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets handle the case where the user is empty
<p-avatar | ||
class="w-6 h-6" | ||
shape="circle" | ||
size="normal" | ||
[image]="environment.latestDeployment.user?.avatarUrl" | ||
[pTooltip]="keycloakService.isCurrentUser(environment.latestDeployment.user?.login) ? 'You' : environment.latestDeployment.user?.name" | ||
[styleClass]="getAvatarBorderClass(environment.latestDeployment.user?.login || '')" | ||
(click)="openUserProfile(environment.latestDeployment.user?.login || '')" | ||
> | ||
</p-avatar> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be better if we make this a component so in pr list or other places we can use the user profile picture component
permissionService = inject(PermissionService); | ||
keycloakService = inject(KeycloakService); | ||
dateService = inject(DateService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better to use private
@@ -93,6 +108,7 @@ export class EnvironmentListViewComponent { | |||
message: `Are you sure you want to deploy to ${environment.name}?`, | |||
accept: () => { | |||
this.deploy.emit(environment); | |||
this.queryClient.invalidateQueries({ queryKey: this.queryKey() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets invalidate query in the parent compoennt 👍🏻
// 4) Combine the lists | ||
List<ActivityHistoryDto> combined = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add HeliosDeployment entities where deploymentId
field is empty, in order to show failed workflowrun's of deployments
It is the case where we don't get the Deployment
entity since the workflowrun failed before the Deployment entity is created
import io.micrometer.common.lang.Nullable; | ||
import java.time.OffsetDateTime; | ||
import org.springframework.lang.NonNull; | ||
|
||
@JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
public record EnvironmentLockHistoryDto( | ||
@NonNull Long id, | ||
String lockedBy, | ||
User lockedBy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets use userdto
FROM EnvironmentLockHistory elh | ||
WHERE elh.environment.id = :environmentId | ||
AND elh.unlockedAt IS NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets also add
elh.environment.enabled = true
private Optional<Deployment> findDeploymentById(Environment environment, Long deploymentId) { | ||
if (deploymentId == null) { | ||
return Optional.empty(); | ||
} | ||
return environment.getDeployments().stream() | ||
.filter(d -> d.getId().equals(deploymentId)) | ||
.findFirst(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove this method
* <p>Returns a small wrapper object with either a real Deployment or a HeliosDeployment. This | ||
* helps unify your "latest" logic into one place. | ||
*/ | ||
public LatestDeploymentUnion findLatestDeployment(Environment env) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better to get latest heliosdeployment and also deployment and then compare which one is the latest by createdAt or updatedAt fields?
Lets imagine a scenario where somebody triggered deployment from Helios and we don't get any webhook data related with that, but then somebody else triggered deployment from GitHub UI and then lets discuss this scenario and what happens in WorklowRun webhook listener, Deployment webhook listener and our final entity status and other fields and what this method returns
Thanks for the heads up! Added the migration script :) |
This PR introduces key improvements to environment visibility, deployment tracking, and user activity transparency. Below are the highlights:
Key changes
1. Environment Locking Enhancements
2. Deployment Visibility Without Expansion
Faster Insights: Users instantly see deployment status, lock details, and deployment history without expanding rows.
3. Real-Time Deployment Status
4. Activity History Enrichment
Technical Improvements
Impact
Screenshots and Recordings
Deployment via Helios
deployment_helios.mov
Deployment via Github UI and monitoring in Helios
deployment_github.mov
Activity History