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

restore community tasks as openshift addons #2427

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

restore community tasks as openshift addons #2427

wants to merge 1 commit into from

Conversation

jkhelil
Copy link
Member

@jkhelil jkhelil commented Nov 27, 2024 •
edited
Loading

Changes

In 1.17 after removing clusterTasks all the community clusterTasks also removed
here we put back community tasks as tasks under openshift-pipelines namespace

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 27, 2024
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 27, 2024
@jkhelil jkhelil changed the title put back community tasks as openshift addonsn put back community tasks as openshift addons Nov 27, 2024
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/community_tasks.go Do not exist 0.0%

@jkhelil
Copy link
Member Author

jkhelil commented Nov 27, 2024

/test pull-tekton-operator-integration-tests

@jkhelil
Copy link
Member Author

jkhelil commented Nov 27, 2024

@savitaashture @PuneetPunamiya ptal

pratap0007
pratap0007 reviewed Nov 27, 2024
View reviewed changes
config/crs/openshift/config/all/operator_v1alpha1_config_cr.yaml Outdated
@@ -27,6 +27,8 @@ spec:
value: "true"
- name: resolverStepActions
value: "true"
- name: communityClusterTasks
Copy link
Contributor

Choose a reason for hiding this comment

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

I think name should be communityTasks

pkg/reconciler/openshift/tektonaddon/community_tasks.go Outdated
@@ -0,0 +1,87 @@
/*
Copyright 2022 The Tekton Authors
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
Copyright 2022 The Tekton Authors
Copyright 2024 The Tekton Authors

@jkhelil jkhelil force-pushed the community_tasks branch 2 times, most recently from dc9939d to 88ef347 Compare November 28, 2024 06:58
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/community_tasks.go Do not exist 0.0%

@pratap0007
Copy link
Contributor

LGTM

@jkhelil
Copy link
Member Author

jkhelil commented Dec 2, 2024

@savitaashture ptal

@savitaashture
Copy link
Contributor

Thank you @jkhelil PR looks good to me
can you try upgrade scenario to make sure nothing is breaking

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

The pull request process is described 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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2024
@jkhelil
Copy link
Member Author

jkhelil commented Dec 9, 2024

@savitaashture I have checked the upgrade scenario and seems working well, no issues.
There is one thing, on 0.73.x, user has on his tektonconfig communityClusterTasks to true, after upgarde, reconcile is failing, until the user move the old communityClusterTasks parameter to communityTasks, otherwise it gets a validating webhook issue, the param communityClusterTasks is nolonger present on the new version

ppitonak
ppitonak reviewed Dec 9, 2024
View reviewed changes
config/crs/openshift/config/all/operator_v1alpha1_config_cr.yaml Outdated
@@ -27,6 +27,8 @@ spec:
value: "true"
- name: resolverStepActions
value: "true"
- name: communityTasks
Copy link
Contributor

Choose a reason for hiding this comment

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

Originally we called it communityClusterTasks, shouldn't it be communityResolverTasks so that we follow established naming conventions?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, make sense as we have resolverTasks resolverStepcActions etc

@jkhelil jkhelil changed the title put back community tasks as openshift addons restore community tasks as openshift addons Dec 11, 2024
@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/community_tasks.go Do not exist 0.0%

@jkhelil
Copy link
Member Author

jkhelil commented Dec 18, 2024

@savitaashture I have checked the upgrade scenario and seems working well, no issues. There is one thing, on 0.73.x, user has on his tektonconfig communityClusterTasks to true, after upgarde, reconcile is failing, until the user move the old communityClusterTasks parameter to communityTasks, otherwise it gets a validating webhook issue, the param communityClusterTasks is nolonger present on the new version

We should consider #2439 to fix the upgrade issue

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/community_tasks.go Do not exist 0.0%

@jkhelil
Copy link
Member Author

jkhelil commented Jan 6, 2025

@savitaashture I have checked the upgrade scenario and seems working well, no issues. There is one thing, on 0.73.x, user has on his tektonconfig communityClusterTasks to true, after upgarde, reconcile is failing, until the user move the old communityClusterTasks parameter to communityTasks, otherwise it gets a validating webhook issue, the param communityClusterTasks is nolonger present on the new version

We should consider #2439 to fix the upgrade issue

fixed now, Upgrade test working well

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/community_tasks.go Do not exist 0.0%

@tekton-robot
Copy link
Contributor

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/community_tasks.go Do not exist 0.0%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pratap0007 pratap0007 pratap0007 left review comments

@ppitonak ppitonak ppitonak left review comments

@vdemeester vdemeester vdemeester approved these changes

@piyush-garg piyush-garg Awaiting requested review from piyush-garg

@PuneetPunamiya PuneetPunamiya Awaiting requested review from PuneetPunamiya

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jkhelil @tekton-robot @pratap0007 @savitaashture @vdemeester @ppitonak