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

Konfluxing #1056

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Konfluxing #1056

merged 9 commits into from
Nov 13, 2024

Conversation

maknop
Copy link
Contributor

@maknop maknop commented Sep 25, 2024

Migrating Clowder to use Konflux for building images. This pipeline will also consist of running the E2E and unit tests.

@maknop
Copy link
Contributor Author

maknop commented Oct 3, 2024

/retest

@maknop maknop force-pushed the new_pipeline branch 4 times, most recently from 010ef06 to bb8ee74 Compare October 15, 2024 19:50
@maknop maknop requested a review from psav October 28, 2024 19:58
Dockerfile Outdated Show resolved Hide resolved
@psav psav requested a review from Victoremepunto November 7, 2024 11:18
Copy link
Contributor

@Victoremepunto Victoremepunto left a comment

Choose a reason for hiding this comment

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

After reviewing the Tekton tasks here, I can't understand why the on-push one is running on every commit push, and why the on-pull-request one is actually not running at all.

I would suggest if possible, to, on a separate PR, simply onboard Clowder as is, and merge those tasks directly (even if they don't build successfully) - and then on this PR, we can rebase on top of it, so that we can see the changes you are adding on this PR (as opposed to, the full Tekton task definition).

@maknop maknop force-pushed the new_pipeline branch 3 times, most recently from f55675d to e85d4ba Compare November 7, 2024 18:40
@Victoremepunto
Copy link
Contributor

@maknop I've reviewed your PR, thank you very much for doing the actual Konflux onboarding first, as a separate PR, and rebased this PR on top, it is now easier to see what you are adding on top in this PR.

However, I've noticed the pipeline you used was incorrect (it was the docker-build one, and I can see you're using trusted artifacts, hence your chosen pipeline should've been the docker-build-oci-ta one instead.

I've fixed that with #1074, please rebase your changes once again, we should see even less LOC changed from your PR. My early comment about the target branch for the trigger is still valid, please do not change it, it needs to be set to master

CC @psav

Copy link
Contributor

@Victoremepunto Victoremepunto left a comment

Choose a reason for hiding this comment

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

I've noticed the tasks are using older SHAs than what's upstream, please revert the ones that are using older than what's upstream

.tekton/clowder-pull-request.yaml Outdated Show resolved Hide resolved
.tekton/clowder-pull-request.yaml Outdated Show resolved Hide resolved
@maknop
Copy link
Contributor Author

maknop commented Nov 8, 2024

/retest

@Victoremepunto
Copy link
Contributor

/retest

@Victoremepunto Victoremepunto self-requested a review November 12, 2024 16:42
Copy link
Contributor

@Victoremepunto Victoremepunto left a comment

Choose a reason for hiding this comment

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

Good job, thanks for adding the changes to the Tekton tasks

@psav this still needs your approval, I have only reviewed the Tekton bits, they look good, we'll consider adding a follow up to add an ITS to run the EC in PRs

@Victoremepunto Victoremepunto dismissed their stale review November 12, 2024 16:47

Requested changes to Tekton tasks have been addressed

Copy link
Collaborator

@psav psav left a comment

Choose a reason for hiding this comment

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

@Victoremepunto and I went through this today for a final review - Regrettably there are some issues that we need to address/fix/explain before this can be merged, however these should be relatively quick to remediate with the right guidance from the team/konflux.

Dockerfile Outdated Show resolved Hide resolved
.tekton/clowder-push.yaml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
ci/minikube_e2e_tests.sh Outdated Show resolved Hide resolved
deps/update_e2e_deps.sh Outdated Show resolved Hide resolved
ci/konflux_minikube_e2e_tests.sh Outdated Show resolved Hide resolved
ci/konflux_minikube_e2e_tests.sh Outdated Show resolved Hide resolved
ci/konflux_minikube_e2e_tests.sh Outdated Show resolved Hide resolved
ci/konflux_minikube_e2e_tests.sh Outdated Show resolved Hide resolved
@psav psav merged commit 7490e46 into master Nov 13, 2024
6 of 9 checks passed
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.

3 participants