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

Jump to bottom

PLNSRVCE-431: Adds annotation to allow skipping the generation of PaC resources #57

Closed
wants to merge 1 commit into from
Closed

PLNSRVCE-431: Adds annotation to allow skipping the generation of PaC resources #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 19 additions & 13 deletions controllers/component_build_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ const (

PartOfLabelName = "app.kubernetes.io/part-of"
PartOfAppStudioLabelValue = "appstudio"

skipPacResourceGenerationAnnotation = "skip-pipelines-as-code-resource-generation"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
skipPacResourceGenerationAnnotation = "skip-pipelines-as-code-resource-generation"
skipPacResourceGenerationAnnotation = "com.redhat.appstudio/skip-pipelines-as-code-resource-generation"

it is better to qualify labels / annotations with a group designation ... my suggestion reuses the group used for the initial build annotation

)

// ComponentBuildReconciler watches AppStudio Component object in order to submit builds
Expand Down Expand Up @@ -216,21 +218,25 @@ func (r *ComponentBuildReconciler) Reconcile(ctx context.Context, req ctrl.Reque
}

// Manage merge request for Pipelines as Code configuration
bundle := gitopsprepare.PrepareGitopsConfig(ctx, r.NonCachingClient, component).BuildBundle
mrUrl, err := ConfigureRepositoryForPaC(component, pacSecret.Data, webhookTargetUrl, webhookSecretString, bundle)
if err != nil {
log.Error(err, "failed to setup repository for Pipelines as Code")
r.EventRecorder.Event(&component, "Warning", "ErrorConfiguringPaCForComponentRepository", err.Error())
return ctrl.Result{}, err
}
var mrMessage string
if mrUrl != "" {
mrMessage = fmt.Sprintf("Pipelines as Code configuration merge request: %s", mrUrl)
if val, ok := component.Annotations[skipPacResourceGenerationAnnotation]; ok && val == "1" {
r.EventRecorder.Event(&component, "Normal", "PipelinesAsCodeConfiguration", "Skipping the generation of .tekton resources")
} else {
mrMessage = "Pipelines as Code configuration is up to date"
bundle := gitopsprepare.PrepareGitopsConfig(ctx, r.NonCachingClient, component).BuildBundle
mrUrl, err := ConfigureRepositoryForPaC(component, pacSecret.Data, webhookTargetUrl, webhookSecretString, bundle)
if err != nil {
log.Error(err, "failed to setup repository for Pipelines as Code")
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed recently in jvm-build-service that with our default level of logging, controller runtime will log the generation of the event in the pod log as well; so you'll end up with essentially duplicate logs with this one ^^ and the event below.

Either in a manual bringup, or investigating the build-service logs collected in the artifacts of the e2e test, you should confirm if that is the case with build-service, and if so, just go with the event here.

@mmorhun FYI

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I can confirm, that events are logged as well and we have duplicates in logs (my mistake before).

r.EventRecorder.Event(&component, "Warning", "ErrorConfiguringPaCForComponentRepository", err.Error())
return ctrl.Result{}, err
}
var mrMessage string
if mrUrl != "" {
mrMessage = fmt.Sprintf("Pipelines as Code configuration merge request: %s", mrUrl)
} else {
mrMessage = "Pipelines as Code configuration is up to date"
}
log.Info(mrMessage)
r.EventRecorder.Event(&component, "Normal", "PipelinesAsCodeConfiguration", mrMessage)
}
log.Info(mrMessage)
r.EventRecorder.Event(&component, "Normal", "PipelinesAsCodeConfiguration", mrMessage)

// Set initial build annotation to prevent recreation of the PaC integration PR
if err := r.Client.Get(ctx, req.NamespacedName, &component); err != nil {
Expand Down
42 changes: 40 additions & 2 deletions controllers/component_build_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ var _ = Describe("Component initial build controller", func() {
github.NewGithubClientByApp = func(appId int64, privateKeyPem []byte, owner string) (*github.GithubClient, error) { return nil, nil }
github.NewGithubClient = func(accessToken string) *github.GithubClient { return nil }

createComponentForPaCBuild(resourceKey)
createComponentForPaCBuild(resourceKey, false)
}, 30)

_ = AfterEach(func() {
Expand Down Expand Up @@ -192,7 +192,7 @@ var _ = Describe("Component initial build controller", func() {
}
createSecret(pacSecretKey, pacSecretNewData)

createComponentForPaCBuild(resourceKey)
createComponentForPaCBuild(resourceKey, false)
setComponentDevfileModel(resourceKey)

ensurePersistentStorageCreated(resourceKey)
Expand Down Expand Up @@ -757,4 +757,42 @@ var _ = Describe("Component initial build controller", func() {
deleteComponentInitialPipelineRuns(componentKey)
})
})

Context("Test skipping of Pipelines as Code resources generation", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to have a separate Context for this test?

_ = BeforeEach(func() {
github.NewGithubClientByApp = func(appId int64, privateKeyPem []byte, owner string) (*github.GithubClient, error) { return nil, nil }
github.NewGithubClient = func(accessToken string) *github.GithubClient { return nil }

createNamespace(pipelinesAsCodeNamespace)
createRoute(pacRouteKey, "pac-host")
pacSecretData := map[string]string{
"github-application-id": "12345",
"github-private-key": githubAppPrivateKey,
}
createSecret(pacSecretKey, pacSecretData)
}, 30)

_ = AfterEach(func() {
deleteComponent(resourceKey)
deleteSecret(pacSecretKey)
deleteRoute(pacRouteKey)
}, 30)

It("should skip the generation of Pipelines as Code resources if the annotation is set to 1", func() {
createComponentForPaCBuild(resourceKey, true)

github.CreatePaCPullRequest = func(g *github.GithubClient, d *github.PaCPullRequestData) (string, error) {
Fail("PR creation should not be invoked")
return "", nil
}

github.SetupPaCWebhook = func(g *github.GithubClient, webhookUrl, webhookSecret, owner, repository string) error {
Fail("Webhook setup should not be invoked")
return nil
}

setComponentDevfileModel(resourceKey)
ensurePaCRepositoryCreated(resourceKey)
})
})
})
7 changes: 6 additions & 1 deletion controllers/suite_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func deleteApplication(resourceKey types.NamespacedName) {
}

// createComponent creates sample component resource and verifies it was properly created
func createComponentForPaCBuild(componentLookupKey types.NamespacedName) {
func createComponentForPaCBuild(componentLookupKey types.NamespacedName, skipPacResourceGeneration bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't go with a parameter here because we need it only in one test. Please recreate component in the test itself as it was done in other specific cases.

component := &appstudiov1alpha1.Component{
TypeMeta: metav1.TypeMeta{
APIVersion: "appstudio.redhat.com/v1alpha1",
Expand All @@ -130,6 +130,11 @@ func createComponentForPaCBuild(componentLookupKey types.NamespacedName) {
},
},
}

if skipPacResourceGeneration {
component.ObjectMeta.Annotations[skipPacResourceGenerationAnnotation] = "1"
}

Expect(k8sClient.Create(ctx, component)).Should(Succeed())

getComponent(componentLookupKey)
Expand Down