256/remove application - refactor code to collect resource information #511
Conversation
There was a problem hiding this comment.
👍 Only nits and questions. As I mentioned in Slack, similar changes around SourceType here: #497
pkg/services/app/add.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("unable to create pull request: %w", err) | ||
| } | ||
| a.logger.Println("Pull Request created: %s\n", prLink.Get().WebURL) | ||
| return nil | ||
| } | ||
|
|
||
| func (a *App) getAppResourceInfo(app wego.Application, clusterName string) *AppResourceInfo { |
There was a problem hiding this comment.
Can this just be a pure function with no receiver? It might signal the intent better.
There was a problem hiding this comment.
Good point. An earlier iteration was referencing something from the app and I neglected to make that change once it no longer did
pkg/services/app/add.go
Outdated
| return result | ||
| } | ||
|
|
||
| func (a *AppResourceInfo) sinkKind() string { |
There was a problem hiding this comment.
What does sink mean in this case?
There was a problem hiding this comment.
Breaking each app automation into a "source" and a "sink" (input from repo, output to cluster). I'm happy to use some other terminology if you have any ideas.
There was a problem hiding this comment.
This appears to be returning an "automation" kind, which we are referring to as a DeploymentType elsewhere. Maybe deployKind() for now?
There was a problem hiding this comment.
Sure, that seems reasonable to me
| TargetDirAutomationPath string | ||
| } | ||
|
|
||
| func dirExists(d string) bool { |
There was a problem hiding this comment.
small nit: this function already exists in utils, works for both files and directories
There was a problem hiding this comment.
Cool. Thanks for pointing that out!
There was a problem hiding this comment.
Actually, after looking... I need to know it's a directory 🤷♂️
| } | ||
|
|
||
| // findAppManifests locates all manifests associated with a specific application within a repository | ||
| func findAppManifests(application wego.Application, repoDir string) ([][]byte, error) { |
There was a problem hiding this comment.
it seems this function is only used in the tests, show it be move it there ? 🤔
There was a problem hiding this comment.
It's only used in tests right now (because I wrote the test for it first :-)). It will be used in the actual code later.
| gitProvider, err := a.gitProviderFactory(params.GitProviderToken) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var secretRef string | ||
| if SourceType(params.SourceType) == SourceTypeGit { | ||
| secretRef, err = a.createAndUploadDeployKey(params, params.Url, clusterName, gitProvider) | ||
| secretRef, err = a.createAndUploadDeployKey(info, params.DryRun, info.Spec.URL, gitProvider) |
There was a problem hiding this comment.
might be a good idea to use info.Spec.URL within createAndUploadDeployKey as we are already passing in info
There was a problem hiding this comment.
We're using that parameter to select either the URL or the ConfigURL and need it to decide which one
pkg/services/app/add.go
Outdated
| a.logger.Actionf("Cloning %s", params.Url) | ||
| if err := a.cloneRepo(params.Url, params.Branch, params.DryRun); err != nil { | ||
| a.logger.Actionf("Cloning %s", info.Spec.URL) | ||
| if err := a.cloneRepo(info.Spec.URL, info.Spec.Branch, params.DryRun); err != nil { |
There was a problem hiding this comment.
would it be a good idea to remove the cloned repo when the user uses --url flag ? The cloning is happening in a temporary folder but I wonder if we should delete that temporary folder right away or if it would be ok to let the system delete it later? 🤔
There was a problem hiding this comment.
Wow. Funny we never noticed that before. 👍
pkg/services/app/add.go
Outdated
| return fmt.Errorf("failed to clone application repo: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if !params.DryRun { | ||
| if !params.AutoMerge { | ||
| if err := a.createPullRequestToRepo(params, gitProvider, ".wego", params.Url, clusterName, appHash, appSpec, appGoat); err != nil { | ||
| if err := a.createPullRequestToRepo(info, gitProvider, info.Spec.URL, appHash, appSpec, appGoat); err != nil { |
There was a problem hiding this comment.
we could probably skip info.Spec.URL as we are passing in info
There was a problem hiding this comment.
Actually, we can't here. This is called in two different places; once with the URL and once with the ConfigURL. We need to know which one to use.
pkg/services/app/remove_test.go
Outdated
| return "", err | ||
| } | ||
|
|
||
| if err := ioutil.WriteFile(filepath.Join(workloadPath1, "nginx.yaml"), []byte(workloadYaml1), 0644); err != nil { |
There was a problem hiding this comment.
it's a minor thing, but we don't need to specify the whole file definition to verify if the Remove is working. you could just pass the string here []byte("file1") and the same later and check it in the test.
I think that makes clear what is being actually tested and clean up the code since we don't actually need the actual definition for anything else.
There was a problem hiding this comment.
That makes sense; you're right, the file contents are irrelevant.
…other uncalled code
jrryjcksn
force-pushed
the
256/remove-application
branch
from
45fbaf2
to
bfb53ee
Compare
What changed?
As I started to consider how to find all the resources to remove, I noticed that the necessary information was spread out through the code and based on AddParams. This collects all information about created resources in an AppResourceInfo structure (derived from a wego.Application) so that it will stay consistent with applications and be available to the remove code (which doesn't have AddParams).
How did you test it?
Other than the first part of remove (which is covered by unit tests), this didn't change behavior and is tested by the existing unit tests.
Release notes
No
Documentation Changes
No