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

feat: add tekton-tasks-operator #532

Merged
merged 7 commits into from
May 22, 2023
Merged

feat: add tekton-tasks-operator #532

merged 7 commits into from
May 22, 2023

Conversation

codingben
Copy link
Member

What this PR does / why we need it:

Add kubevirt/tekton-tasks-operator here to have fewer operators and reduce maintenance efforts.

Release note:

Add tekton-tasks-operator

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 28, 2023
@codingben codingben marked this pull request as draft March 28, 2023 16:06
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2023
@codingben codingben mentioned this pull request Mar 28, 2023
Closed
ksimon1
ksimon1 reviewed Mar 28, 2023
View reviewed changes
internal/common/labels.go Outdated Show resolved Hide resolved
@codingben
Copy link
Member Author

@ksimon1 I'd prefer to keep everything in one commit until I'll have no errors and CI passed, to not deal with updating multiple commits. Once it's ready to be merged, I'll split PR into commits. Sounds good?

@ksimon1
Copy link
Member

ksimon1 commented Mar 29, 2023

Fine by me

@akrejcir
Copy link
Collaborator

Is the TektonTasks API going to be a separate CRD, that will be watched by the SSP operator? If yes, then it needs a separate controller.

@ksimon1
Copy link
Member

ksimon1 commented Mar 29, 2023

Is the TektonTasks API going to be a separate CRD, that will be watched by the SSP operator? If yes, then it needs a separate controller.

No, we will insert it inside already existing CRD

@akrejcir
Copy link
Collaborator

Do we need to keep the API definitions of TektonTasks for backward compatibility?

@codingben codingben requested review from ksimon1 and removed request for 0xFelix March 30, 2023 15:29
@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 31, 2023
@codingben
Copy link
Member Author

Can you please review the functional tests? they should be already in /tests/* directory.

@lyarwood
Copy link
Member

lyarwood commented Apr 4, 2023

/cc

lyarwood
lyarwood reviewed Apr 5, 2023
View reviewed changes
api/v1beta1/ssp_types.go Outdated Show resolved Hide resolved
@codingben
Copy link
Member Author

FYI, I've rebased this PR on latest tekton-tasks-operator changes [1] and rebased on latest master changes [2]. Also make lint passed now.

[1] kubevirt/tekton-tasks-operator@be9190e
[2] d12622e

akrejcir
akrejcir reviewed Apr 17, 2023
View reviewed changes
Copy link
Collaborator

@akrejcir akrejcir left a comment

Choose a reason for hiding this comment

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

I looked at this only partially.

Before you post this for proper review, can you split it to multiple commits, so that the first commit will be only a simple copy-paste of code from TTO repo, and the subsequent commits will contain other changes. Then it will be easier to see what has changed from TTO.

api/v1beta1/ssp_types.go Outdated Show resolved Hide resolved
automation/common/example-pipelines-test.sh Outdated Show resolved Hide resolved
automation/common/versions.sh Outdated Show resolved Hide resolved
config/crd/bases/tektontasks.kubevirt.io_tektontasks.yaml Outdated Show resolved Hide resolved
controllers/ssp_controller.go Outdated Show resolved Hide resolved
internal/common/environment_test.go Outdated Show resolved Hide resolved
internal/operands/tekton-pipelines/tekton-pipelines.go Show resolved Hide resolved
internal/operands/tekton-tasks/tekton-tasks.go Show resolved Hide resolved
internal/tekton-bundle/tekton-bundle.go Outdated Show resolved Hide resolved
@codingben
Copy link
Member Author

codingben commented Apr 17, 2023 •
edited
Loading

Hi @akrejcir, a quick question, you've mentioned: so that the first commit will be only a simple copy-paste of code from TTO repo.

There is no simple copy-paste because it requires some code changes, in order to work in ssp-operator (especially functional tests). What I want to do is:

  • feat: update SSP api
  • feat: add tekton-bundle
  • feat: add tekton-pipelines operand
  • feat: add tekton-tasks operand
  • test: add tekton functional tests
  • other commits

Sounds good?

@akrejcir
Copy link
Collaborator

akrejcir commented Apr 17, 2023 •
edited
Loading

I've checked the TTO code, and it is mostly compatible with SSP code, so copy-paste should be possible, even if the commit does not compile by itself. I think it will make review easier, if I can see what code was copied from TTO and what code is new.

I would propose to split it to commits like this:

  1. Copy code related only to tekton-tasks operand from TTO to SSP. This would also include functional tests and any common code used by the operand, like tekton-bundle.
  2. Add a new field to the SSPSpec in the api package, and modify the tekton-tasks code so that it compiles, and passes unittests. Ideally it should pass functests too, but that would take a long time to run tests.
  3. Copy code related to tekton-pipelines operand from TTO to SSP, including functional tests.
  4. Add new filed to the SSPSpec, and fix the code, so that it compiles and passes unit tests.
  5. Copy anything that's left, like csv-generator and CI scripts.
  6. Modify them if needed, so that they compile and work correctly.
  7. Any other refactoring to make the code simpler and easier to read.

It will be some work, but it will be easier to review and spot potential bugs.

@kubevirt-bot
Copy link
Contributor

@codingben: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test e2e-upgrade-functests

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.

akrejcir
akrejcir reviewed May 18, 2023
View reviewed changes
internal/operands/tekton-pipelines/tekton-pipelines.go Show resolved Hide resolved
internal/operands/tekton-pipelines/tekton-pipelines.go Show resolved Hide resolved
internal/operands/tekton-pipelines/tekton-pipelines.go Outdated Show resolved Hide resolved
internal/operands/tekton-pipelines/tekton-pipelines.go Outdated Show resolved Hide resolved
tests/tekton-pipelines_test.go Outdated Show resolved Hide resolved
internal/operands/tekton-tasks/tekton-tasks.go Outdated Show resolved Hide resolved
internal/operands/tekton-pipelines/tekton-pipelines.go Outdated Show resolved Hide resolved
internal/operands/tekton-pipelines/tekton-pipelines.go Outdated Show resolved Hide resolved
internal/operands/tekton-tasks/tekton-tasks.go Outdated Show resolved Hide resolved
internal/operands/tekton-tasks/tekton-tasks.go Outdated Show resolved Hide resolved
@codingben codingben marked this pull request as draft May 18, 2023 12:47
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2023
@codingben codingben marked this pull request as ready for review May 18, 2023 14:49
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2023
@codingben codingben requested a review from akrejcir May 18, 2023 14:50
@ksimon1 ksimon1 mentioned this pull request May 19, 2023
Merged
@codingben
fix: tekton-pipelines operand compilation
e817f09 
Fix compilation errors and run unit tests
successfully. Also, rewriting functional tests
to work in ssp-operator.

Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@codingben
src: add tekton-tasks operand
4be7470 
Copy-paste tekton-tasks operand and
related code from Tekton Tasks Operator. It includes:

- Operand itself
- Common
- Bundle
- Functional tests

Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@codingben
fix: tekton-tasks operand compilation
6cb14a1 
Fix compilation errors and run unit tests
successfully. Also, rewriting functional tests
to work in ssp-operator.

Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@codingben
src: add csv-generator
b453cb3 
Copy-paste csv-generator from Tekton Tasks
Operator and adjust it to work in ssp-operator,
including tests.

Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@codingben
src: add automation tools
4a97c13 
Copy-paste automation tools from Tekton Tasks
Operator and adjust them to work in ssp-operator.

Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@codingben
docs: add tekton tasks and pipelines
bbb5ead 
Add info about the two new Tekton operands and
required CRDs in order to use them.

Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented May 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codingben
Copy link
Member Author

/test e2e-functests

@kubevirt-bot
Copy link
Contributor

@codingben: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test e2e-functests

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.

@codingben
Copy link
Member Author

@akrejcir @ksimon1 All tests passed, your /approve is very much appreciated :)

@ksimon1
Copy link
Member

ksimon1 commented May 22, 2023

/lgtm
The comments I posted can be fixed in a follow up PRs

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2023
@akrejcir
Copy link
Collaborator

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2023
@akrejcir akrejcir mentioned this pull request May 22, 2023
Closed
@kubevirt-bot kubevirt-bot merged commit 5971e68 into kubevirt:master May 22, 2023
@codingben codingben deleted the CNV-21854 branch May 22, 2023 08:35
@codingben codingben mentioned this pull request Aug 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ksimon1 ksimon1 ksimon1 left review comments

@lyarwood lyarwood Awaiting requested review from lyarwood

@0xFelix 0xFelix Awaiting requested review from 0xFelix

@akrejcir akrejcir Awaiting requested review from akrejcir

Assignees

@ksimon1 ksimon1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@codingben @ksimon1 @akrejcir @lyarwood @kubevirt-bot @0xFelix