Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions cmd/action.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package main

import (
"errors"

"github.com/konveyor/tackle2-hub/api"
)

// Action provides and addon action.
type Action interface {
Run(*Data) error
}

// NewAction returns and action based the data provided.
func NewAction(d *Data) (a Action, err error) {
switch d.Action {
case "fetch":
a = &Fetch{}
case "import":
a = &Import{}
case "generate":
a = &Generate{}
default:
err = errors.New("action not supported")
}
return
}

// BaseAction provides base functionality.
type BaseAction struct {
application api.Application
platform api.Platform
}

// setApplication fetches and sets `application` referenced by the task.
// The associated `platform` will be set when as appropriate.
func (r *BaseAction) setApplication() (err error) {
defer func() {
if err != nil {
if errors.Is(err, &api.NotFound{}) {
err = nil
}
}
}()
app, err := addon.Task.Application()
if err == nil {
r.application = *app
} else {
return
}
if app.Platform == nil {
return
}
p, err := addon.Platform.Get(app.Platform.ID)
if err == nil {
r.platform = *p
}
return
}
Comment on lines +37 to +59
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify error handling and avoid masking legitimate errors.

The deferred error handling pattern is overly complex and may mask legitimate errors. The current implementation suppresses all NotFound errors, which might not always be appropriate.

-func (r *BaseAction) setApplication() (err error) {
-	defer func() {
-		if err != nil {
-			if errors.Is(err, &api.NotFound{}) {
-				err = nil
-			}
-		}
-	}()
+func (r *BaseAction) setApplication() (err error) {
 	app, err := addon.Task.Application()
-	if err == nil {
-		r.application = *app
-	} else {
+	if err != nil {
+		if errors.Is(err, &api.NotFound{}) {
+			return nil // Application not found is acceptable
+		}
 		return
 	}
+	r.application = *app
+	
 	if app.Platform == nil {
 		return
 	}
 	p, err := addon.Platform.Get(app.Platform.ID)
-	if err == nil {
-		r.platform = *p
+	if err != nil {
+		if errors.Is(err, &api.NotFound{}) {
+			return nil // Platform not found is acceptable
+		}
+		return
 	}
+	r.platform = *p
 	return
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (r *BaseAction) setApplication() (err error) {
defer func() {
if err != nil {
if errors.Is(err, &api.NotFound{}) {
err = nil
}
}
}()
app, err := addon.Task.Application()
if err == nil {
r.application = *app
} else {
return
}
if app.Platform == nil {
return
}
p, err := addon.Platform.Get(app.Platform.ID)
if err == nil {
r.platform = *p
}
return
}
func (r *BaseAction) setApplication() (err error) {
app, err := addon.Task.Application()
if err != nil {
if errors.Is(err, &api.NotFound{}) {
return nil // Application not found is acceptable
}
return
}
r.application = *app
if app.Platform == nil {
return
}
p, err := addon.Platform.Get(app.Platform.ID)
if err != nil {
if errors.Is(err, &api.NotFound{}) {
return nil // Platform not found is acceptable
}
return
}
r.platform = *p
return
}
🤖 Prompt for AI Agents
In cmd/action.go lines 37 to 59, simplify the error handling by removing the
deferred function that suppresses NotFound errors. Instead, handle errors
explicitly where they occur, returning them directly without masking. This
avoids hiding legitimate errors and makes the error flow clearer and easier to
maintain.


// setPlatform fetches and sets `platform` referenced by the task.
func (r *BaseAction) setPlatform() (err error) {
defer func() {
if err != nil {
if errors.Is(err, &api.NotFound{}) {
err = nil
}
}
}()
p, err := addon.Task.Platform()
if err == nil {
r.platform = *p
}
return
}
Comment on lines +61 to +75
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply same error handling simplification to setPlatform.

The setPlatform method has the same complex error handling pattern that should be simplified for consistency.

-func (r *BaseAction) setPlatform() (err error) {
-	defer func() {
-		if err != nil {
-			if errors.Is(err, &api.NotFound{}) {
-				err = nil
-			}
-		}
-	}()
+func (r *BaseAction) setPlatform() (err error) {
 	p, err := addon.Task.Platform()
-	if err == nil {
-		r.platform = *p
+	if err != nil {
+		if errors.Is(err, &api.NotFound{}) {
+			return nil // Platform not found is acceptable
+		}
+		return
 	}
+	r.platform = *p
 	return
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In cmd/action.go lines 61 to 75, simplify the error handling in the setPlatform
method by removing the deferred function that checks and clears the error for
NotFound. Instead, directly handle the error after calling
addon.Task.Platform(), setting err to nil if it matches api.NotFound, and assign
r.platform only if no error occurs. This will make the error handling consistent
and clearer.


// selectIdentity selects an identity based on kind.
func (r *BaseAction) selectIdentity(kind string) (ref *api.Ref, err error) {
idapi := addon.Application.Identity(r.application.ID)
id, found, err := idapi.Find(kind)
if err != nil {
return
}
if found {
ref = &api.Ref{
ID: id.ID,
Name: id.Name,
}
}
return
}
163 changes: 163 additions & 0 deletions cmd/cloudfoundry/provider.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
package cloudfoundry

import (
"path/filepath"

cf "github.com/cloudfoundry/go-cfclient/v3/config"
cfp "github.com/konveyor/asset-generation/pkg/providers/discoverers/cloud_foundry"
hub "github.com/konveyor/tackle2-hub/addon"
"github.com/konveyor/tackle2-hub/api"
"github.com/konveyor/tackle2-hub/api/jsd"
"github.com/konveyor/tackle2-hub/migration/json"
)

var (
addon = hub.Addon
)

// Provider is a cloudfoundry provider.
type Provider struct {
URL string
Identity *api.Identity
}

// Fetch the manifest for the application.
func (p *Provider) Fetch(application *api.Application) (m *api.Manifest, err error) {
if application.Coordinates == nil {
return
}
Comment on lines +26 to +28
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add nil check for application coordinates.

The method returns silently when coordinates are nil. Consider returning an error to make the failure explicit.

 	if application.Coordinates == nil {
+		err = fmt.Errorf("application coordinates not defined")
 		return
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if application.Coordinates == nil {
return
}
if application.Coordinates == nil {
err = fmt.Errorf("application coordinates not defined")
return
}
🤖 Prompt for AI Agents
In cmd/cloudfoundry/provider.go around lines 22 to 24, the code currently
returns silently if application.Coordinates is nil. Modify this to return an
explicit error indicating that the coordinates are nil, so the failure is clear
to the caller. Update the function signature if needed to support returning an
error and ensure the error message clearly states the issue with missing
coordinates.

coordinates := Coordinates{}
err = application.Coordinates.As(&coordinates)
if err != nil {
return
}
ref := cfp.AppReference{
SpaceName: coordinates.Space,
AppName: coordinates.Name,
}
client, err := p.client(ref.SpaceName)
if err != nil {
return
}
manifest, err := client.Discover(ref)
if err != nil {
return
}
m = &api.Manifest{}
m.Content = manifest.Content
m.Secret = manifest.Secret
return
}

// Find applications on the platform.
func (p *Provider) Find(filter api.Map) (found []api.Application, err error) {
f := Filter{}
err = filter.As(&f)
if err != nil {
return
}
client, err := p.client(f.Spaces...)
if err != nil {
return
}
spaces, err := client.ListApps()
if err != nil {
return
}
schema, err := addon.Schema.Find("platform", "cloudfoundry", "coordinates")
if err != nil {
return
}
for space, applications := range spaces {
if !f.MatchSpace(space) {
continue
}
for _, ref := range applications {
appRef, cast := ref.(cfp.AppReference)
if !cast {
continue
}
if !f.MatchName(appRef.AppName) {
continue
}
r := api.Application{}
r.Name = appRef.AppName
r.Coordinates = &jsd.Document{
Schema: schema.Name,
Content: json.Map{
"name": appRef.AppName,
"space": space,
},
}
found = append(found, r)
}
}
return
}

// client returns a cloudfoundry client.
func (p *Provider) client(spaces ...string) (client *cfp.CloudFoundryProvider, err error) {
options := []cf.Option{
cf.SkipTLSValidation(),
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Make TLS validation configurable.

TLS validation is always skipped, which is a security risk. This should be configurable based on the environment.

-	options := []cf.Option{
-		cf.SkipTLSValidation(),
-	}
+	options := []cf.Option{}
+	// Only skip TLS validation if explicitly configured
+	if p.Identity != nil && p.Identity.Settings != nil {
+		if skipTLS, ok := p.Identity.Settings["skipTLS"].(bool); ok && skipTLS {
+			options = append(options, cf.SkipTLSValidation())
+		}
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cf.SkipTLSValidation(),
options := []cf.Option{}
// Only skip TLS validation if explicitly configured
if p.Identity != nil && p.Identity.Settings != nil {
if skipTLS, ok := p.Identity.Settings["skipTLS"].(bool); ok && skipTLS {
options = append(options, cf.SkipTLSValidation())
}
}
🤖 Prompt for AI Agents
In cmd/cloudfoundry/provider.go at line 86, TLS validation is currently always
skipped by calling cf.SkipTLSValidation(), which poses a security risk. Modify
the code to make TLS validation configurable by introducing a configuration
option or environment variable that controls whether TLS validation is skipped
or enforced. Use this configuration to conditionally call cf.SkipTLSValidation()
only when skipping TLS validation is explicitly enabled.

}
if p.Identity != nil {
options = append(
options,
cf.UserPassword(
p.Identity.User,
p.Identity.Password))
}
cfConfig, err := cf.New(p.URL, options...)
if err != nil {
return
}
pConfig := &cfp.Config{
CloudFoundryConfig: cfConfig,
SpaceNames: spaces,
}
client, err = cfp.New(pConfig, &addon.Log, true)
if err != nil {
return
}
return
}

// Coordinates - platform coordinates.
type Coordinates struct {
Space string `json:"space"`
Name string `json:"name"`
}

// Filter applications.
type Filter struct {
Spaces []string `json:"spaces"`
Names []string `json:"names"`
}

// MatchSpace returns true when the application name matches the filter.
func (f *Filter) MatchSpace(name string) (match bool) {
for _, s := range f.Spaces {
match = s == name
if match {
break
}
}
return
}
Comment on lines +137 to +146
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle empty filter to match all spaces.

When the Spaces slice is empty, the method returns false instead of matching all spaces. This may not be the intended behavior.

 func (f *Filter) MatchSpace(name string) (match bool) {
+	if len(f.Spaces) == 0 {
+		return true // No filter means match all
+	}
 	for _, s := range f.Spaces {
 		match = s == name
 		if match {
 			break
 		}
 	}
 	return
 }
🤖 Prompt for AI Agents
In cmd/cloudfoundry/provider.go around lines 124 to 133, the MatchSpace method
currently returns false when the Spaces slice is empty, but it should match all
spaces in that case. Modify the method to first check if the Spaces slice is
empty and return true immediately to indicate a match for all spaces. Otherwise,
proceed with the existing loop to check for a matching space name.


// MatchName returns true when the name matches the filter.
// The name may be a glob.
func (f *Filter) MatchName(name string) (match bool) {
var err error
for _, pattern := range f.Names {
match, err = filepath.Match(pattern, name)
if err != nil {
addon.Log.Error(err, "Invalid glob pattern", "pattern", pattern)
continue
}
if match {
break
}
}
return
}
68 changes: 68 additions & 0 deletions cmd/fetch.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package main

import (
"errors"

cf "github.com/konveyor/tackle2-addon-platform/cmd/cloudfoundry"
"github.com/konveyor/tackle2-hub/api"
)

// Fetch application manifest action.
type Fetch struct {
BaseAction
}

// Run executes the action.
func (a *Fetch) Run(d *Data) (err error) {
err = a.setApplication()
if err != nil {
return
}
addon.Activity(
"[Fetch] Fetch manifest for application (id=%d): %s",
a.application.ID,
a.application.Name)
addon.Activity(
"[Fetch] Using platform (id=%d): %s",
a.platform.ID,
a.platform.Name)
Comment on lines +25 to +28
Copy link

@coderabbitai coderabbitai bot Jul 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing setPlatform() call causes potential runtime panic.

The code logs platform information but never calls setPlatform() to populate the platform context. This will likely cause a runtime panic when accessing a.platform.ID and a.platform.Name.

 addon.Activity(
 	"[Fetch] Fetch manifest for application (id=%d): %s",
 	a.application.ID,
 	a.application.Name)
+err = a.setPlatform()
+if err != nil {
+	return
+}
 addon.Activity(
 	"[Fetch] Using platform (id=%d): %s",
 	a.platform.ID,
 	a.platform.Name)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
addon.Activity(
"[Fetch] Using platform (id=%d): %s",
a.platform.ID,
a.platform.Name)
addon.Activity(
"[Fetch] Fetch manifest for application (id=%d): %s",
a.application.ID,
a.application.Name)
err = a.setPlatform()
if err != nil {
return
}
addon.Activity(
"[Fetch] Using platform (id=%d): %s",
a.platform.ID,
a.platform.Name)
🤖 Prompt for AI Agents
In cmd/fetch.go around lines 23 to 26, the code logs platform details using
a.platform.ID and a.platform.Name without first calling setPlatform(), which can
cause a runtime panic due to uninitialized platform data. To fix this, ensure
that setPlatform() is called before accessing a.platform fields to properly
initialize the platform context and avoid nil pointer dereference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setApplication() will set the a.platform as needed with the platform associated with the application.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

@jortel Thank you for the clarification! Let me verify the implementation to understand the behavior better.

🐰
╭─────╮
│ ◔ ◔ │
│ ⌄ │ curious about the code details!
╰─────╯


🏁 Script executed:

#!/bin/bash
# Find the setApplication method implementation
ast-grep --pattern 'func ($_ *BaseAction) setApplication() error {
  $$$
}'

Length of output: 77


🏁 Script executed:

#!/bin/bash
# Search for setApplication method with ripgrep
rg "setApplication" -A 10 -B 2

# Also search for setPlatform to understand the relationship
rg "setPlatform" -A 10 -B 2

Length of output: 2629


🏁 Script executed:

#!/bin/bash
# Read the action.go file to see the BaseAction methods
cat cmd/action.go

Length of output: 1746


@jortel You're absolutely right! I can see from the setApplication() implementation that it does indeed set both the application and the platform:

// setApplication fetches and sets `application` referenced by the task.
// The associated `platform` will be set when as appropriate.
func (r *BaseAction) setApplication() (err error) {
    // ... error handling ...
    app, err := addon.Task.Application()
    if err == nil {
        r.application = *app
    } else {
        return
    }
    if app.Platform == nil {
        return
    }
    p, err := addon.Platform.Get(app.Platform.ID)
    if err == nil {
        r.platform = *p  // Platform is set here!
    }
    return
}

The method first fetches the application, then checks if the application has an associated platform, and if so, fetches and sets the platform. The comment even explicitly states "The associated platform will be set when as appropriate."

Thank you for the correction! The original suggestion to add setPlatform() was indeed unnecessary.

🐰
╭─────╮
│ ◔ ◔ │
│ ⌄ │ grateful for the clarification!
╰─────╯


🧬 Code Graph Analysis Results

Relevant snippets for understanding setApplication() and platform association in cmd/fetch.go:


File: cmd/action.go
Lines: 30-33

BaseAction struct {
	application api.Application
	platform    api.Platform
}

This snippet defines the BaseAction struct which holds the application and platform fields. Since Fetch embeds BaseAction, these fields are accessible in Fetch. The setApplication() method likely sets these fields, including a.platform as mentioned in the user comment.


File: cmd/cloudfoundry/provider.go
Lines: 15-18

Provider struct {
	URL      string
	Identity *api.Identity
}

Defines the Provider struct used in Fetch.Run() to fetch manifests from a platform. The Provider is initialized with the platform URL and optionally an identity. This is relevant because a.platform is used to create this provider, so understanding the platform structure and its usage is helpful.


File: cmd/main.go
Lines: 27-35

Data struct {
	// Action
	// - import
	// - fetch
	// - generate
	Action string `json:"action"`
	// Filter applications.
	Filter api.Map `json:"filter"`
}

Defines the Data struct passed into Fetch.Run(). While not directly related to setApplication(), it provides context on the input data structure for the action.


These snippets provide context on the application and platform fields in Fetch, the Provider struct that uses platform info, and the input data structure. The actual setApplication() method implementation is not included here but would be part of the same or related files to see how a.platform is set based on the application.


✏️ Learnings added
Learnt from: jortel
PR: konveyor/tackle2-addon-platform#2
File: cmd/fetch.go:23-26
Timestamp: 2025-07-06T19:09:45.429Z
Learning: In the cmd/fetch.go file, the setApplication() method on BaseAction will automatically set the a.platform field with the platform associated with the application, so there's no need to call setPlatform() separately.

Learnt from: jortel
PR: konveyor/tackle2-addon-platform#2
File: cmd/fetch.go:23-26
Timestamp: 2025-07-06T19:09:45.429Z
Learning: In the BaseAction struct, the setApplication() method automatically sets both the application and platform fields. When called, it first fetches the application, then checks if the application has an associated platform (app.Platform != nil), and if so, fetches the platform using addon.Platform.Get(app.Platform.ID) and sets r.platform = *p. This means there's no need to call setPlatform() separately when setApplication() is used.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

var manifest *api.Manifest
switch a.platform.Kind {
case "cloudfoundry":
manifest, err = a.cloudfoundry()
if err != nil {
return
}
default:
err = errors.New("platform.kind not supported")
return
}
manifest.Application.ID = a.application.ID
err = addon.Manifest.Create(manifest)
if err == nil {
addon.Activity(
"Manifest (id=%d) created.",
manifest.ID)
}
return
}

// cloudfoundry implementation.
func (a *Fetch) cloudfoundry() (manifest *api.Manifest, err error) {
p := cf.Provider{
URL: a.platform.URL,
}
if a.platform.Identity.ID != 0 {
p.Identity, err = addon.Identity.Get(a.platform.Identity.ID)
if err == nil {
addon.Activity(
"[Fetch] Using credentials (id=%d): %s",
p.Identity.ID,
p.Identity.Name)
} else {
return
}
}
manifest, err = p.Fetch(&a.application)
return
}
Loading