Skip to content

Conversation

raghavyuva
Copy link
Owner

No description provided.

Copy link
Contributor

coderabbitai bot commented Sep 2, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/project_labels

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@raghavyuva raghavyuva changed the title Feat/project labels feat: add labels to projects Sep 2, 2025
Copy link
Collaborator

@zhravan zhravan left a comment

Choose a reason for hiding this comment

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

Overall LGTM, minor suggestions

@@ -360,6 +360,7 @@ func (router *Router) DeployApplicationRoutes(f *fuego.Server, deployController
fuego.Get(f, "/logs/{application_id}", deployController.GetLogs)
fuego.Get(f, "/deployments/{deployment_id}/logs", deployController.GetDeploymentLogs)
fuego.Get(f, "/deployments", deployController.GetApplicationDeployments)
fuego.Put(f, "/labels", deployController.UpdateApplicationLabels)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally better to keep it as /application/{appID}/labels right?
query string would make sense if it was optional value, but in our case this is mandatory

)

func (s *DeployService) UpdateApplicationLabels(applicationID uuid.UUID, labels []string, organizationID uuid.UUID) error {
return s.storage.UpdateApplicationLabels(applicationID, labels, organizationID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove duplicate strings just incase if its there in labels array? Like sanity before storing in db? In FE, should we keep max character limit otherwise might be able to add a big string, looking odd in the UI?

setNewLabel('');
return;
}
const updated = [...localLabels, value];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const updated = [...localLabels, value];
const updated = [...new Set([...localLabels, value])];

removes duplication

type UpdateOptions struct {
Force bool
ForceWithoutCache bool
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
}


generatePorts := "seq 49152 65535"

getUsedPorts := "command -v ss >/dev/null 2>&1 && ss -tan | awk '{print $4}' | cut -d':' -f2 | grep '[0-9]\\{1,5\\}' | sort -u || netstat -tan | awk '{print $4}' | grep ':[0-9]' | cut -d':' -f2 | sort -u"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we move this to utils?

@raghavyuva raghavyuva self-assigned this Sep 9, 2025
@zhravan zhravan deleted the branch master September 10, 2025 18:02
@zhravan zhravan closed this Sep 10, 2025
@raghavyuva raghavyuva reopened this Sep 14, 2025
@raghavyuva raghavyuva changed the base branch from feat/develop to master September 14, 2025 16:36
@raghavyuva raghavyuva changed the base branch from master to feat/develop September 14, 2025 16:36
@raghavyuva
Copy link
Owner Author

reopened accidental closure of this PR, @zhravan for your notice

@zhravan zhravan deleted the branch master September 16, 2025 20:37
@zhravan zhravan closed this Sep 16, 2025
@zhravan zhravan reopened this Sep 16, 2025
Base automatically changed from feat/develop to master September 19, 2025 16:19
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