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.

Sign up for GitHub

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

[DEVHAS-488]annotation to force generate gitops #410

Merged
merged 10 commits into from
Nov 15, 2023
Merged

[DEVHAS-488]annotation to force generate gitops #410

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d82ae25
add force annotation check
yangcao77 Nov 7, 2023
a0f0a28
rebase
yangcao77 Nov 7, 2023
d7350e8
set annotation back to false once generation successfully completed
yangcao77 Nov 7, 2023
5a02383
rebase
yangcao77 Nov 13, 2023
a94bbac
address review comments
yangcao77 Nov 13, 2023
bb4bb62
update message and time
yangcao77 Nov 14, 2023
047dd4c
add test
yangcao77 Nov 14, 2023
83d06a4
rebase
yangcao77 Nov 14, 2023
c6c10f4
clean up
yangcao77 Nov 14, 2023
c2ab406
rebase
yangcao77 Nov 14, 2023
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
4 changes: 2 additions & 2 deletions controllers/application_controller_finalizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func containsString(slice []string, s string) bool {
return false
}

// getApplicationFailCount gets the given counter annotation on the resource (defaults to 0 if unset)
// getCounterAnnotation gets the given counter annotation on the resource (defaults to 0 if unset)
func getCounterAnnotation(annotation string, obj client.Object) (int, error) {
objAnnotations := obj.GetAnnotations()
if objAnnotations == nil || objAnnotations[annotation] == "" {
Expand All @@ -100,7 +100,7 @@ func getCounterAnnotation(annotation string, obj client.Object) (int, error) {
return strconv.Atoi(counterAnnotation)
}

// setApplicationFailCount sets the given counter annotation on the resource to the specified value
// setCounterAnnotation sets the given counter annotation on the resource to the specified value
func setCounterAnnotation(annotation string, obj client.Object, count int) {
objAnnotations := obj.GetAnnotations()
if objAnnotations == nil {
Expand Down
32 changes: 30 additions & 2 deletions controllers/component_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"os"
"path/filepath"
"reflect"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -78,6 +79,7 @@
applicationFailCounterAnnotation = "applicationFailCounter"
maxApplicationFailCount = 5
componentName = "Component"
forceGenerationAnnotation = "forceGitopsGeneration"
)

//+kubebuilder:rbac:groups=appstudio.redhat.com,resources=components,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -131,6 +133,13 @@
}

setCounterAnnotation(applicationFailCounterAnnotation, &component, 0)
forceGenerateGitopsResource := false
forceGenerateGitopsResource, err = getForceGenerateGitopsAnnotation(component)
if err != nil {
log.Info("failed to get forceGeneration annotation, set forceGenerateGitopsResource to false")
forceGenerateGitopsResource = false
}

Check warning on line 141 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L139-L141 

Added lines #L139 - L141 were not covered by tests
log.Info(fmt.Sprintf("forceGenerateGitopsResource is %v", forceGenerateGitopsResource))

ghClient, err := r.GitHubTokenClient.GetNewGitHubClient("")
if err != nil {
Expand Down Expand Up @@ -180,7 +189,7 @@
log.Info(fmt.Sprintf("added the finalizer %v", req.NamespacedName))
}
} else {
if hasApplication.Status.Devfile != "" && len(component.Status.Conditions) > 0 && component.Status.Conditions[len(component.Status.Conditions)-1].Status == metav1.ConditionTrue && containsString(component.GetFinalizers(), compFinalizerName) {
if hasApplication.Status.Devfile != "" && (forceGenerateGitopsResource || len(component.Status.Conditions) > 0 && component.Status.Conditions[len(component.Status.Conditions)-1].Status == metav1.ConditionTrue && containsString(component.GetFinalizers(), compFinalizerName)) {
// only attempt to finalize and update the gitops repo if an Application is present & the previous Component status is good
// A finalizer is present for the Component CR, so make sure we do the necessary cleanup steps
metrics.ComponentDeletionTotalReqs.Inc()
Expand Down Expand Up @@ -217,7 +226,7 @@
isUpdateConditionPresent := false
isGitOpsRegenSuccessful := false
for _, condition := range component.Status.Conditions {
if condition.Type == "GitOpsResourcesGenerated" && condition.Reason == "GenerateError" && condition.Status == metav1.ConditionFalse {
if forceGenerateGitopsResource || (condition.Type == "GitOpsResourcesGenerated" && condition.Reason == "GenerateError" && condition.Status == metav1.ConditionFalse) {
log.Info(fmt.Sprintf("Re-attempting GitOps generation for %s", component.Name))
// Parse the Component Devfile
compDevfileData, err := cdqanalysis.ParseDevfileWithParserArgs(&devfileParser.ParserArgs{Data: []byte(component.Status.Devfile), Token: gitToken})
Expand All @@ -234,6 +243,7 @@
return ctrl.Result{}, err
} else {
log.Info(fmt.Sprintf("GitOps re-generation successful for %s", component.Name))
setForceGenerateGitopsAnnotation(&component, "false")

Check warning on line 246 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L246 

Added line #L246 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

The person or the tool that sets the annotations can handle un setting the annotation. I don't think we need a dedicated function for setting it to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking the person or HAC can enable the force generation, but they are not going to track it? and after the regeneration is done, should this be set to false? or the expected usage will be after enabling, the gitops resource going to attempt regeneration in every following reconcile?

Copy link
Member

Choose a reason for hiding this comment

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

the expected usage will be after enabling, the gitops resource going to attempt regeneration in every following reconcile

I think the expected usage should be that as long as the annotation is present, every reconcile will re-generate the gitops resources. It's up to the user or tool to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have discussed in team meeting, setting the annotation to false when a force generation is done

err := r.SetGitOpsGeneratedConditionAndUpdateCR(ctx, req, &component, nil)
if err != nil {
return ctrl.Result{}, err
Expand Down Expand Up @@ -308,8 +318,8 @@
if gitToken == "" {
gitURL, err = cdqanalysis.ConvertGitHubURL(source.GitSource.URL, source.GitSource.Revision, context)
if err != nil {
// ConvertGitHubURL only returns user error
metrics.ImportGitRepoSucceeded.Inc()

Check warning on line 322 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L321-L322 

Added lines #L321 - L322 were not covered by tests
log.Error(err, fmt.Sprintf("Unable to convert Github URL to raw format, exiting reconcile loop %v", req.NamespacedName))
_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
return ctrl.Result{}, err
Expand All @@ -331,21 +341,21 @@
// Use SPI to retrieve the devfile from the private repository
devfileBytes, devfileLocation, err = spi.DownloadDevfileUsingSPI(r.SPIClient, ctx, component, gitURL, source.GitSource.Revision, context)
if err != nil {
// Increment the import git repo failed metric on non-user errors
if _, ok := err.(*devfile.NoFileFound); !ok {
metrics.ImportGitRepoFailed.Inc()
} else {
metrics.ImportGitRepoSucceeded.Inc()
}
log.Error(err, fmt.Sprintf("Unable to download from any known devfile locations from %s %v", gitURL, req.NamespacedName))
_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
return ctrl.Result{}, err

Check warning on line 352 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L344-L352 

Added lines #L344 - L352 were not covered by tests
}
metrics.ImportGitRepoSucceeded.Inc()

gitURL, err := cdqanalysis.ConvertGitHubURL(source.GitSource.URL, source.GitSource.Revision, context)
if err != nil {
log.Error(err, fmt.Sprintf("Unable to convert Github URL to raw format, exiting reconcile loop %v", req.NamespacedName))

Check warning on line 358 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L358 

Added line #L358 was not covered by tests
_ = r.SetCreateConditionAndUpdateCR(ctx, req, &component, err)
return ctrl.Result{}, err
}
Expand Down Expand Up @@ -610,16 +620,16 @@
if err != nil {
retErr := err
if strings.Contains(strings.ToLower(err.Error()), "github push protection") {
retErr = fmt.Errorf("potential secret leak caught by github push protection")
// to get the URL token
// e.g. <GitURL>/security/secret-scanning/unblock-secret/2WlUv72plUf05tgshlpRLzSlH4R \n
var unblockURL string
splited := strings.Split(strings.ToLower(err.Error()), "unblock-secret/")
if len(splited) > 1 {
token := strings.Split(splited[1], " ")[0]
unblockURL = fmt.Sprintf("%v/security/secret-scanning/unblock-secret/%v", component.Status.GitOps.RepositoryURL, token)
log.Error(retErr, fmt.Sprintf("unable to generate gitops resources due to git push protecton error, follow the link to unblock the secret: %v", unblockURL))
}

Check warning on line 632 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L623-L632 

Added lines #L623 - L632 were not covered by tests
} else {
log.Error(retErr, "unable to generate gitops resources due to error")
}
Expand All @@ -631,23 +641,23 @@
metrics.ControllerGitRequest.With(prometheus.Labels{"controller": componentName, "tokenName": ghClient.TokenName, "operation": "CommitAndPush"}).Inc()
err = r.Generator.CommitAndPush(tempDir, "", gitOpsURL, mappedGitOpsComponent.Name, gitOpsBranch, "Generating GitOps resources")
if err != nil {
retErr := err
if strings.Contains(strings.ToLower(err.Error()), "github push protection") {
retErr = fmt.Errorf("potential secret leak caught by github push protection")
// to get the URL token
// e.g. <GitURL>/security/secret-scanning/unblock-secret/2WlUv72plUf05tgshlpRLzSlH4R \n
var unblockURL string
splited := strings.Split(strings.ToLower(err.Error()), "unblock-secret/")
if len(splited) > 1 {
token := strings.Split(splited[1], " ")[0]
unblockURL = fmt.Sprintf("%v/security/secret-scanning/unblock-secret/%v", component.Status.GitOps.RepositoryURL, token)
log.Error(retErr, fmt.Sprintf("unable to commit and push gitops resources due to git push protecton error, follow the link to unblock the secret: %v", unblockURL))
}
} else {
log.Error(retErr, "unable to commit and push gitops resources due to error")
}

Check warning on line 658 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L644-L658 

Added lines #L644 - L658 were not covered by tests
ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
return retErr

Check warning on line 660 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L660 

Added line #L660 was not covered by tests
}

// Get the commit ID for the gitops repository
Expand Down Expand Up @@ -754,3 +764,21 @@
return ctrl.Result{}, componentErr
}
}

// getForceGenerateGitopsAnnotation gets the internal annotation on the component whether to force generate the gitops resource
func getForceGenerateGitopsAnnotation(component appstudiov1alpha1.Component) (bool, error) {
Copy link
Member

@johnmcollier johnmcollier Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We can simplify this a fair bit by avoiding casting the string to bool (which means we also don't need to return an err)

func getForceGenerateGitopsAnnotation(component appstudiov1alpha1.Component) bool {
    if compAnnotations != nil && compAnnotations[forceGenerationAnnotation] == "true" {
        return true
    }
    return false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

compAnnotations := component.GetAnnotations()
if compAnnotations == nil || compAnnotations[forceGenerationAnnotation] == "" {
return false, nil
} else {
return strconv.ParseBool(compAnnotations[forceGenerationAnnotation])
}

Check warning on line 775 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L774-L775 

Added lines #L774 - L775 were not covered by tests
}

func setForceGenerateGitopsAnnotation(component *appstudiov1alpha1.Component, value string) {
compAnnotations := component.GetAnnotations()
if compAnnotations == nil {
compAnnotations = make(map[string]string)
}
compAnnotations[forceGenerationAnnotation] = value

Check warning on line 783 in controllers/component_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/component_controller.go#L778-L783 

Added lines #L778 - L783 were not covered by tests
}
Loading