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

FBC updates #541

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from
Open

FBC updates #541

wants to merge 9 commits into from

Conversation

spotlesstofu
Copy link
Contributor

To complete KATA-3577.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 18, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@spotlesstofu
Copy link
Contributor Author

spotlesstofu commented Feb 27, 2025 •
edited
Loading

This is ready. I'll rebase and squash after #552 is merged. v4.18 will come in a new pull.
@littlejawa @lmilleri if you could review please.

@spotlesstofu spotlesstofu marked this pull request as ready for review February 27, 2025 14:09
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2025
@openshift-ci openshift-ci bot requested review from bpradipt and snir911 February 27, 2025 14:14
littlejawa
littlejawa reviewed Feb 27, 2025
View reviewed changes
fbc/render.sh Outdated

test -n "$OCP_VERSIONS" || OCP_VERSIONS=$(ls v4.*)

BUILD_REGISTRY="quay.io/redhat-user-workloads/ose-osc-tenant/trustee/"
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
BUILD_REGISTRY="quay.io/redhat-user-workloads/ose-osc-tenant/trustee/"
BUILD_REGISTRY="quay.io/redhat-user-workloads/ose-osc-tenant/"

littlejawa
littlejawa reviewed Feb 27, 2025
View reviewed changes
.tekton/osc-operator-pull-request.yaml
pipelinesascode.tekton.dev/on-cel-expression:
event == "pull_request" &&
target_branch == "devel" &&
files.all.exists(path, !path.matches('.tekton/.*fbc-.*|fbc/.*))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do the same for the other pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the other pipelines start only when their files are modified ("allow list")

only the operator pipeline uses a "deny list"

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2025
@littlejawa
Copy link
Contributor

I'll rebase and squash after #552 is merged

Maybe that's what you meant, but just to make sure:
Do not squash everything into a single commit. From what I see, I think you can group them by topic (duplicate, render, etc), and a separate one to add the generated files.
Introducing them as separate commits helps reviewing, but also in the future if we want to look up something specific about one of the scripts, we don't want to review one huge commit.

lmilleri
lmilleri reviewed Mar 3, 2025
View reviewed changes
fbc/render.sh Outdated
test -n "$OCP_VERSIONS" || OCP_VERSIONS=$(ls v4.*)

BUILD_REGISTRY="quay.io/redhat-user-workloads/ose-osc-tenant/trustee/"
RELEASE_REGISTRY="registry.redhat.io/confidential-compute-attestation-tech-preview/"
Copy link

Choose a reason for hiding this comment

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

replace confidential-compute-attestation-tech-preview?

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2025
@spotlesstofu
Copy link
Contributor Author

I addressed all the comments

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Mar 4, 2025

@spotlesstofu: all tests passed!

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-sigs/prow repository. I understand the commands that are listed here.

littlejawa
littlejawa approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@littlejawa littlejawa left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks @spotlesstofu

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2025
@spotlesstofu
Copy link
Contributor Author

Please merge when you see fit. I don't have write access.

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

@lmilleri lmilleri lmilleri left review comments

@littlejawa littlejawa littlejawa approved these changes

@bpradipt bpradipt Awaiting requested review from bpradipt

@snir911 snir911 Awaiting requested review from snir911

Assignees

@littlejawa littlejawa

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spotlesstofu @littlejawa @lmilleri @openshift-merge-robot