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

✨ Kai support #631

Merged
merged 6 commits into from
Jun 11, 2024
Merged

✨ Kai support #631

merged 6 commits into from
Jun 11, 2024

Conversation

jortel
Copy link
Contributor

@jortel jortel commented Apr 29, 2024

closes #630

  • Added endpoint:
    • GET /applications/:id/analyses/issues
    • GET /analyses
    • GET /analyses/:id/issues
  • Updated api.Analysis to omit issues, summary. Struct never used this way.
  • Add field: Analysis.Commit. migration=13.

Added IssueWriter to support new endpoints.


migration/14

diff -bur migration/v13/model/analysis.go migration/v14/model/analysis.go
--- migration/v13/model/analysis.go	2024-06-11 03:29:34.402853992 -0700
+++ migration/v14/model/analysis.go	2024-06-11 04:29:29.898613621 -0700
@@ -6,7 +6,8 @@
 type Analysis struct {
 	Model
 	Effort        int
-	Archived      bool             `json:"archived"`
+	Commit        string
+	Archived      bool
 	Summary       JSON             `gorm:"type:json"`
 	Issues        []Issue          `gorm:"constraint:OnDelete:CASCADE"`
 	Dependencies  []TechDependency `gorm:"constraint:OnDelete:CASCADE"`
diff -bur migration/v13/model/application.go migration/v14/model/application.go
--- migration/v13/model/application.go	2024-06-11 03:29:34.402853992 -0700
+++ migration/v14/model/application.go	2024-06-11 04:29:29.898613621 -0700
@@ -76,7 +76,7 @@
 	return
 }
 
-// Validation Hook to avoid cyclic dependencies.
+// BeforeCreate detects cyclic dependencies.
 func (r *Dependency) BeforeCreate(db *gorm.DB) (err error) {
 	var nextDeps []*Dependency
 	var nextAppsIDs []uint
@@ -96,11 +96,11 @@
 	return
 }
 
-// Custom error type to allow API recognize Cyclic Dependency error and assign proper status code.
+// DependencyCyclicError reports cyclic Dependency error.
 type DependencyCyclicError struct{}
 
-func (err DependencyCyclicError) Error() string {
-	return "cyclic dependencies are not allowed"
+func (e DependencyCyclicError) Error() string {
+	return "Cyclic dependencies are not permitted."
 }
 
 type BusinessService struct {

type Analysis struct {
Model
Effort int
Commit string `json:"commit"`
Copy link
Contributor Author

@jortel jortel May 8, 2024

Choose a reason for hiding this comment

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

hint: added Commit.

EnvAppName = "APP_NAME"
EnvDisconnected = "DISCONNECTED"
EnvAnalysisReportPath = "ANALYSIS_REPORT_PATH"
EnvAnalysisArchiverEnabled = "ANALYSIS_ARCHIVER_ENABLED"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: added EnvAnalysisArchiverEnabled.

if err != nil {
_ = ctx.Error(err)
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: Moved this code blocker here to use r.Model() to properly transfer analysis fields in the posted resource (like: Commit).

@jortel jortel marked this pull request as ready for review May 29, 2024 20:01
@jortel jortel added this to the v0.5.0 milestone Jun 5, 2024
type Analysis struct {
Model
Effort int
Commit string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: adds commit.

// @tags analyses
// @produce json
// @success 200 {object} []api.Analysis
// @router /analyses [get]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: added for completeness.

// @produce octet-stream
// @success 204 {object}
// @router /analyses/{id}/archive [post]
// @param id path int true "Analysis ID"
Copy link
Contributor Author

@jortel jortel Jun 11, 2024

Choose a reason for hiding this comment

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

hint: added to support manual archiving (by kai service) when automatic archiver disabled.

@@ -185,6 +279,8 @@ func (h AnalysisHandler) AppList(ctx *gin.Context) {
db := h.DB(ctx)
db = db.Model(&model.Analysis{})
db = db.Where("ApplicationID = ?", id)
db = db.Preload("Application")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: preload application so name is set in ref.

@@ -388,7 +482,7 @@ func (h AnalysisHandler) AppCreate(ctx *gin.Context) {
}

db = h.DB(ctx)
db = db.Preload(clause.Associations)
db = db.Preload("Application")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: only preload app. Don't need/want issues.

// @produce json
// @success 200 {object} []api.Issue
// @router /analyses/{id}/issues [get]
// @param id path int true "Analysis ID"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hint: needed by kai to fetch issues.

Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Copy link
Collaborator

@mansam mansam left a comment

Choose a reason for hiding this comment

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

LGTM, just little nits

// @summary Archive an analysis (report) by ID.
// @description Archive an analysis (report) by ID.
// @tags analyses
// @produce octet-stream
Copy link
Collaborator

Choose a reason for hiding this comment

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

@produce line can be removed

// @description Archive an analysis (report) by ID.
// @tags analyses
// @produce octet-stream
// @success 204 {object}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be @success 204 as there's no returned object

@@ -202,15 +298,11 @@ func (h AnalysisHandler) AppList(ctx *gin.Context) {
}
list = append(list, m)
}
err = h.WithCount(ctx, cursor.Count())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is WithCount not necessary here to set the pagination header?

@@ -1683,6 +1936,16 @@ func (h *AnalysisHandler) appIDs(ctx *gin.Context, f qf.Filter) (q *gorm.DB) {
}

// analysisIDs provides analysis IDs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be good to fix up the method name in the commend here

@@ -1900,6 +2158,7 @@ type Issue struct {
// With updates the resource with the model.
func (r *Issue) With(m *model.Issue) {
r.Resource.With(&m.Model)
r.Analysis = m.AnalysisID
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if these should be refs rather than bare IDs, unless these IDs aren't intended to be used to reference API resources?

"gorm.io/gorm"
)

var log = logr.WithName("migration|v13")
Copy link
Collaborator

Choose a reason for hiding this comment

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

log message needs to be updated to v14

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

Successfully merging this pull request may close these issues.

✨ Support AI Service
2 participants