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.

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

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

Conversation

brunoapimentel
Copy link
Contributor

In order to migrate the components in the infra-deployments repository to use HAS Application/Component CRs, we need to avoid having the build-service spamming PRs for each time a new Component is created when a new cluster is bootrstraped.

This PR allows this behavior to be skipped in case an annotation is set in the related Component CR.

When a Component is created, the build-service checks for the related
Git repository to create/update the resources in the .tekton folder if
needed.

Skipping it is necessary in cases of repositories in which we want to
manage the pipelines-as-code configuration manually.

Signed-off-by: Bruno Pimentel <bpimente@redhat.com>
@openshift-ci openshift-ci bot requested review from goldmann and psturc August 25, 2022 19:00
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brunoapimentel
Once this PR has been reviewed and has the lgtm label, please assign gabemontero for approval by writing /assign @gabemontero in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

just a coule of minor items @brunoapimentel

@@ -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

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).

@@ -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.

@@ -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?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 28, 2022

@brunoapimentel: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/build-service-e2e 98c2ca6 link true /test build-service-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Michkov Michkov closed this Feb 17, 2023
@brunoapimentel brunoapimentel deleted the skip-pac-resources-annotation branch August 2, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants