chore(env): env show should show deployed jobs
#3316
Conversation
| @@ -246,6 +253,27 @@ func (d *EnvDescriber) filterDeployedSvcs() ([]*config.Workload, error) { | |||
| return deployedSvcs, nil | |||
| } | |||
|
|
|||
| // filterDeployedJobs lists the jobs that are deployed on the given app and environment | |||
There was a problem hiding this comment.
nit: it's nicee to have a description but function comment is not required if the function is not exported.
internal/pkg/describe/env.go
Outdated
| func (d *EnvDescriber) filterDeployedJobs() ([]*config.Workload, error) { | ||
| allJobs, err := d.configStore.ListJobs(d.app) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list services for app %s: %w", d.app, err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("list services for app %s: %w", d.app, err) | |
| return nil, fmt.Errorf("list jobs for app %s: %w", d.app, err) |
| for _, job := range allJobs { | ||
| jobs[job.Name] = job | ||
| } | ||
| deployedJobNames, err := d.deployStore.ListDeployedJobs(d.app, d.env.Name) |
There was a problem hiding this comment.
hmm we don't necessarily address this in this particular PR but in the near future we should refactor this so that listDeployedWorkloads of deployStore returns []*config.Workload instead of []string. This will simplify the logic of filterDeployedSvcs and filterDeployedJobs.
There was a problem hiding this comment.
yes totally agree, I guess then we don't need to make 2 separate calls to bring []*config.Workload and then []string and add custom logic to filter them.
| @@ -274,6 +302,15 @@ func (e *EnvDescription) HumanString() string { | |||
| fmt.Fprintf(writer, " %s\t%s\n", svc.Name, svc.Type) | |||
| } | |||
| writer.Flush() | |||
| fmt.Fprint(writer, color.Bold.Sprint("\nJobs\n\n")) | |||
There was a problem hiding this comment.
Do you think we want to only show the Jobs section only when there is/are job(s) deployed (btw I think what you have now is fine too)
There was a problem hiding this comment.
I guess it is okay for the awareness of the customers that Jobs are also being showed as a part of env show. And also it aligns with the current implementation for services.
| @@ -35,12 +35,14 @@ type ConfigStoreSvc interface { | |||
| ListEnvironments(appName string) ([]*config.Environment, error) | |||
| ListServices(appName string) ([]*config.Workload, error) | |||
| GetWorkload(appName string, name string) (*config.Workload, error) | |||
| ListJobs(appName string) ([]*config.Workload, error) | |||
| } | |||
|
|
|||
| // DeployedEnvServicesLister wraps methods of deploy store. | |||
| type DeployedEnvServicesLister interface { | |||
There was a problem hiding this comment.
What do you think of renaming this to DeployedEnvWorkloadsLister?
There was a problem hiding this comment.
I thought of doing so but there are a lot of places where we need to change if we change it to DeployedEnvWorkloadsLister. for example, impacted places are -
- env.go
- env_test.go
- lb_web_service.go
- lb_web_service_test.go
- mock_service.go
- rd_web_service.go
- rd_web_service_test.go
- service.go
- worker_service.go
If we really be touching so many files as a part of env show implementation?
Lou1415926
added
the
do-not-merge
paragbhingre
removed
the
do-not-merge
Lou1415926
changed the title
env show should show deployed jobs
This PR adds functionality to show jobs on
env showcommandThis PR addresses #3237
User interface -
