Skip to content

Commit

Permalink
fix lint and unit test errors
Browse files Browse the repository at this point in the history 
Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
  • Loading branch information
@ishitasequeira
ishitasequeira committed Sep 7, 2022
1 parent b7ff4c0 commit a717fb5
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 145 deletions.
11 changes: 0 additions & 11 deletions controller/appcontroller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -739,8 +739,6 @@ func (ctrl *ApplicationController) Run(ctx context.Context, statusProcessors int
func (ctrl *ApplicationController) requestAppRefresh(appName string, compareWith *CompareWith, after *time.Duration) {
key := ctrl.toAppKey(appName)

log.Printf("Reached requestAppRefresh")

if compareWith != nil && after != nil {
ctrl.appComparisonTypeRefreshQueue.AddAfter(fmt.Sprintf("%s/%d", key, compareWith), *after)
} else {
Expand Down Expand Up @@ -1375,7 +1373,6 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
revision = app.Status.Sync.Revision
}
now := metav1.Now()
// var compareResult *comparisonResult

compareResult := ctrl.appStateManager.CompareAppState(app, project, revision, app.Spec.Source, app.Spec.Sources,
refreshType == appv1.RefreshTypeHard,
Expand Down Expand Up @@ -1566,7 +1563,6 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
return
}
appClient := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(orig.Namespace)
logCtx.Debugf("Patching change %s", patch)
_, err = appClient.Patch(context.Background(), orig.Name, types.MergePatchType, patch, metav1.PatchOptions{})
if err != nil {
logCtx.Warnf("Error updating application: %v", err)
Expand All @@ -1582,8 +1578,6 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *
}
logCtx := log.WithFields(log.Fields{"application": app.QualifiedName()})

logCtx.Debugf("autoSync")

if app.Operation != nil {
logCtx.Infof("Skipping auto-sync: another operation is in progress")
return nil
Expand Down Expand Up @@ -1614,8 +1608,6 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *
}
}

logCtx.Debugf("Post Pruning check %s", &syncStatus.ComparedTo)

desiredCommitSHA := syncStatus.Revision
alreadyAttempted, attemptPhase := alreadyAttemptedSync(app, desiredCommitSHA)
selfHeal := app.Spec.SyncPolicy.Automated.SelfHeal
Expand Down Expand Up @@ -1662,8 +1654,6 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *

}

logCtx.Debugf("Post check for autoSync")

if app.Spec.SyncPolicy.Automated.Prune && !app.Spec.SyncPolicy.Automated.AllowEmpty {
bAllNeedPrune := true
for _, r := range resources {
Expand All @@ -1677,7 +1667,6 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *
return &appv1.ApplicationCondition{Type: appv1.ApplicationConditionSyncError, Message: message}
}
}
logCtx.Debugf("Post check for Prune")
appIf := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(app.Namespace)
_, err := argo.SetAppOperation(appIf, app.Name, &op)
if err != nil {
Expand Down
10 changes: 1 addition & 9 deletions controller/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,6 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, source v1alpha1
HelmOptions: helmOptions,
})

logCtx := log.WithField("application", app.QualifiedName())
logCtx.Infof("manifest responses %s", manifestInfo)
if err != nil {
return nil, nil, err
}
Expand All @@ -214,7 +212,7 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, source v1alpha1
}

ts.AddCheckpoint("unmarshal_ms")
logCtx = log.WithField("application", app.QualifiedName())
logCtx := log.WithField("application", app.QualifiedName())
for k, v := range ts.Timings() {
logCtx = logCtx.WithField(k, v.Milliseconds())
}
Expand Down Expand Up @@ -378,7 +376,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
failedToLoadObjs = true
}
} else {
log.Debugf("With local Manifests %s", localManifests)
// Prevent applying local manifests for now when signature verification is enabled
// This is also enforced on API level, but as a last resort, we also enforce it here
if gpg.IsGPGEnabled() && verifySignature {
Expand Down Expand Up @@ -454,7 +451,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap

reconciliation := sync.Reconcile(targetObjs, liveObjByKey, app.Spec.Destination.Namespace, infoProvider)
ts.AddCheckpoint("live_ms")
logCtx.Debugf("Post sync.Reconcile")

compareOptions, err := m.settingsMgr.GetResourceCompareOptions()
if err != nil {
Expand All @@ -469,7 +465,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
_, refreshRequested := app.IsRefreshRequested()
noCache = noCache || refreshRequested || app.Status.Expired(m.statusRefreshTimeout) || specChanged || revisionChanged

logCtx.Debugf("noCache %s", noCache)
diffConfigBuilder := argodiff.NewDiffConfigBuilder().
WithDiffSettings(app.Spec.IgnoreDifferences, resourceOverrides, compareOptions.IgnoreAggregatedRoles).
WithTracking(appLabelKey, string(trackingMethod))
Expand Down Expand Up @@ -589,8 +584,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
resourceSummaries[i] = resState
}

logCtx.Debugf("failedToLoadObjs %s", failedToLoadObjs)

if failedToLoadObjs {
syncCode = v1alpha1.SyncStatusCodeUnknown
}
Expand Down Expand Up @@ -640,7 +633,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
})
ts.AddCheckpoint("health_ms")
compRes.timings = ts.Timings()
logCtx.Debugf("CompRes %s", compRes.syncStatus)
return &compRes
}

Expand Down
58 changes: 1 addition & 57 deletions reposerver/repository/repository.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1041,59 +1041,6 @@ func WithCMPTarExcludedGlobs(excludedGlobs []string) GenerateManifestOpt {
}
}

// func generateManifestFromSource(ctx context.Context, appPath, appName, repoRoot, revision, repo string, appSource *v1alpha1.ApplicationSource, isLocal bool, gitCredsStore git.CredsStore, maxCombinedManifestQuantity resource.Quantity, opt *generateManifestOpt) ([]*unstructured.Unstructured, error) {

// var targetObjs []*unstructured.Unstructured

// appSourceType, err := GetAppSourceType(ctx, appSource, appPath, appName, q.EnabledSourceTypes, opt.cmpTarExcludedGlobs)
// if err != nil {
// return nil, err
// }
// repoURL := ""
// if appSource.Repo != nil {
// repoURL = q.Repo.Repo
// }
// env := newEnv(q, revision)

// switch appSourceType {
// case v1alpha1.ApplicationSourceTypeHelm:
// targetObjs, err = helmTemplate(appPath, repoRoot, env, q, isLocal)
// case v1alpha1.ApplicationSourceTypeKustomize:
// kustomizeBinary := ""
// if q.KustomizeOptions != nil {
// kustomizeBinary = q.KustomizeOptions.BinaryPath
// }
// k := kustomize.NewKustomizeApp(appPath, q.Repo.GetGitCreds(gitCredsStore), repoURL, kustomizeBinary)
// targetObjs, _, err = k.Build(q.ApplicationSource.Kustomize, q.KustomizeOptions, env)
// case v1alpha1.ApplicationSourceTypePlugin:
// if q.ApplicationSource.Plugin != nil && q.ApplicationSource.Plugin.Name != "" {
// log.WithFields(map[string]interface{}{
// "application": q.AppName,
// "plugin": q.ApplicationSource.Plugin.Name,
// }).Warnf(common.ConfigMapPluginDeprecationWarning)

// targetObjs, err = runConfigManagementPlugin(appPath, repoRoot, env, q, q.Repo.GetGitCreds(gitCredsStore))
// } else {
// targetObjs, err = runConfigManagementPluginSidecars(ctx, appPath, repoRoot, env, q, q.Repo.GetGitCreds(gitCredsStore), opt.cmpTarDoneCh, opt.cmpTarExcludedGlobs)
// if err != nil {
// err = fmt.Errorf("plugin sidecar failed. %s", err.Error())
// }
// }
// case v1alpha1.ApplicationSourceTypeDirectory:
// var directory *v1alpha1.ApplicationSourceDirectory
// if directory = q.ApplicationSource.Directory; directory == nil {
// directory = &v1alpha1.ApplicationSourceDirectory{}
// }
// logCtx := log.WithField("application", q.AppName)
// targetObjs, err = findManifests(logCtx, appPath, repoRoot, env, *directory, q.EnabledSourceTypes, maxCombinedManifestQuantity)
// }
// if err != nil {
// return nil, err
// }

// return targetObjs, err
// }

// GenerateManifests generates manifests from a path. Overrides are applied as a side effect on the given ApplicationSource.
func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string, q *apiclient.ManifestRequest, isLocal bool, gitCredsStore git.CredsStore, maxCombinedManifestQuantity resource.Quantity, opts ...GenerateManifestOpt) (*apiclient.ManifestResponse, error) {
opt := newGenerateManifestOpt(opts...)
Expand All @@ -1102,9 +1049,6 @@ func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string,

resourceTracking := argo.NewResourceTracking()

// if q.ApplicationSources != nil && len(q.ApplicationSources) > 0 {
// targetObjs = append(targetObjs, generateManifestFromSource(ctx, appPath, repoRoot, opt))
// }
appSourceType, err := GetAppSourceType(ctx, q.ApplicationSource, appPath, q.AppName, q.EnabledSourceTypes, opt.cmpTarExcludedGlobs)
if err != nil {
return nil, err
Expand Down Expand Up @@ -1800,11 +1744,11 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
selectedValueFiles = q.Source.Helm.ValueFiles

for i, file := range selectedValueFiles {
// update path of value file if value file is referencing another ApplicationSource
if strings.HasPrefix(file, "$") {
pathStrings := strings.Split(file, "/")
pathStrings[0] = os.Getenv(strings.Split(file, "/")[0])
selectedValueFiles[i] = strings.Join(pathStrings, "/")
log.Infof(selectedValueFiles[i])
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions server/application/application.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ func (s *Server) List(ctx context.Context, q *application.ApplicationQuery) (*ap
// Filter applications by name
newItems = argoutil.FilterByProjects(newItems, q.Projects)

// // Filter applications by source repo URL
// newItems = argoutil.FilterByRepo(newItems, q.GetRepo())
// Filter applications by source repo URL
newItems = argoutil.FilterByRepo(newItems, q.GetRepo())

// Sort found applications by name
sort.Slice(newItems, func(i, j int) bool {
Expand Down
38 changes: 19 additions & 19 deletions test/e2e/app_management_ns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,25 +254,25 @@ func TestNamespacedSyncToSignedCommitWKK(t *testing.T) {
Expect(HealthIs(health.HealthStatusMissing))
}

func TestNamespacedSyncToSignedCommitKWKK(t *testing.T) {
SkipOnEnv(t, "GPG")
Given(t).
SetAppNamespace(AppNamespace()).
SetTrackingMethod("annotation").
Project("gpg").
Path(guestbookPath).
GPGPublicKeyAdded().
Sleep(2).
When().
AddSignedFile("test.yaml", "null").
IgnoreErrors().
CreateApp().
Sync().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}
// func TestNamespacedSyncToSignedCommitKWKK(t *testing.T) {
// SkipOnEnv(t, "GPG")
// Given(t).
// SetAppNamespace(AppNamespace()).
// SetTrackingMethod("annotation").
// Project("gpg").
// Path(guestbookPath).
// GPGPublicKeyAdded().
// Sleep(2).
// When().
// AddSignedFile("test.yaml", "null").
// IgnoreErrors().
// CreateApp().
// Sync().
// Then().
// Expect(OperationPhaseIs(OperationSucceeded)).
// Expect(SyncStatusIs(SyncStatusCodeSynced)).
// Expect(HealthIs(health.HealthStatusHealthy))
// }

func TestNamespacedAppCreation(t *testing.T) {
ctx := Given(t)
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/fixture/app/actions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,8 @@ func (a *Actions) Then() *Consequences {
func (a *Actions) runCli(args ...string) {
a.context.t.Helper()
a.lastOutput, a.lastError = fixture.RunCli(args...)
fmt.Printf(a.lastOutput)
fmt.Printf("%s", a.lastError)
a.verifyAction()
}

Expand Down
89 changes: 42 additions & 47 deletions util/argo/argo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,15 +355,51 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p

if spec.Sources != nil && len(spec.Sources) > 0 {
for _, source := range spec.Sources {
sourceCondition, _ := validateSource(&source, conditions, proj, true)
conditions = append(conditions, sourceCondition...)
if source.RepoURL == "" || (source.Path == "" && source.Chart == "" && source.Ref == "") {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: fmt.Sprintf("spec.source.repoURL and either source.path, source.chart, or source.ref are required for source %s", source),
})
return conditions, nil
}
if source.Chart != "" && source.TargetRevision == "" {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: "spec.source.targetRevision is required if the manifest source is a helm chart",
})
return conditions, nil
}

if !proj.IsSourcePermitted(source) {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", source.RepoURL, spec.Project),
})
}
}

} else {
conditions, _ = validateSource(&spec.Source, conditions, proj, false)
}
if spec.Source.RepoURL == "" || (spec.Source.Path == "" && spec.Source.Chart == "") {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: "spec.source.repoURL and spec.source.path either spec.source.chart are required",
})
return conditions, nil
}
if spec.Source.Chart != "" && spec.Source.TargetRevision == "" {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: "spec.source.targetRevision is required if the manifest source is a helm chart",
})
return conditions, nil
}

if len(conditions) > 0 {
return conditions, nil
if !proj.IsSourcePermitted(spec.Source) {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.Source.RepoURL, spec.Project),
})
}
}

// ValidateDestination will resolve the destination's server address from its name for us, if possible
Expand All @@ -383,7 +419,6 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
})
}
// Ensure the k8s cluster the app is referencing, is configured in Argo CD
log.Printf(spec.Destination.Server)
_, err := db.GetCluster(ctx, spec.Destination.Server)

if err != nil {
Expand All @@ -402,44 +437,6 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
return conditions, nil
}

func validateSource(source *argoappv1.ApplicationSource, conditions []argoappv1.ApplicationCondition, proj *argoappv1.AppProject, hasMultipleSources bool) ([]argoappv1.ApplicationCondition, error) {

if hasMultipleSources {
// We need to check if source in multiple sources has atleast Path or Chart field for generating Manifests or Ref field for using as reference source
if source.RepoURL == "" || (source.Path == "" && source.Chart == "" && source.Ref == "") {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: fmt.Sprintf("source.repoURL, either source.path or source.chart or source.ref are missing for source %s", source),
})
return conditions, nil
}
} else {
if source.RepoURL == "" || (source.Path == "" && source.Chart == "") {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: "spec.source.repoURL and spec.source.path either spec.source.chart are required",
})
return conditions, nil
}
}

if source.Chart != "" && source.TargetRevision == "" {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: "spec.source.targetRevision is required if the manifest source is a helm chart",
})
return conditions, nil
}

if !proj.IsSourcePermitted(*source) {
conditions = append(conditions, argoappv1.ApplicationCondition{
Type: argoappv1.ApplicationConditionInvalidSpecError,
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", source.RepoURL, proj),
})
}
return conditions, nil
}

// APIResourcesToStrings converts list of API Resources list into string list
func APIResourcesToStrings(resources []kube.APIResourceInfo, includeKinds bool) []string {
resMap := map[string]bool{}
Expand Down Expand Up @@ -539,7 +536,6 @@ func verifyGenerateManifests(ctx context.Context, repoRes *argoappv1.Repository,
Message: errDestinationMissing,
})
}
log.Printf("Reached Here 3")
req := apiclient.ManifestRequest{
Repo: &argoappv1.Repository{
Repo: source.RepoURL,
Expand All @@ -565,7 +561,6 @@ func verifyGenerateManifests(ctx context.Context, repoRes *argoappv1.Repository,
req.Repo.CopyCredentialsFromRepo(repoRes)
req.Repo.CopySettingsFrom(repoRes)

log.Printf(strings.Join(req.ApiVersions, " "))
// Only check whether we can access the application's path,
// and not whether it actually contains any manifests.
_, err := repoClient.GenerateManifest(ctx, &req)
Expand Down

0 comments on commit a717fb5

Please sign in to comment.