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

OCPVE-647: annotate manifests with capability name #594

Merged
merged 4 commits into from Dec 7, 2023

Conversation

qJkee
Copy link
Contributor

@qJkee qJkee commented Aug 28, 2023

add CloudCredential capability name to all manifests to make component optional

Also, i had to modify the render function. Now, if capability is disabled, we do render nothing

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 28, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 28, 2023

@qJkee: This pull request references OCPVE-647 which is a valid jira issue.

In response to this:

add CloudCredential capability name to all manifests to make component optional

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.

@openshift-ci openshift-ci bot requested review from dlom and lleshchi August 28, 2023 12:11
@abutcher
Copy link
Member

/retest-required

@qJkee qJkee changed the title OCPVE-647: annotate manifests with capability name [WIP] OCPVE-647: annotate manifests with capability name Oct 9, 2023
@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 Oct 9, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 9, 2023

@qJkee: This pull request references OCPVE-647 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

add CloudCredential capability name to all manifests to make component optional

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.

@jstuever
Copy link
Contributor

/cc

@openshift-ci openshift-ci bot requested a review from jstuever October 18, 2023 18:41
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2023
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #594 (2a56e33) into master (c721882) will increase coverage by 0.45%.
Report is 6 commits behind head on master.
The diff coverage is 66.66%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
+ Coverage   48.42%   48.87%   +0.45%     
==========================================
  Files          96       96              
  Lines       11741    12044     +303     
==========================================
+ Hits         5686     5887     +201     
- Misses       5425     5511      +86     
- Partials      630      646      +16     
Files Coverage Δ
pkg/assets/bootstrap/bindata.go 23.85% <ø> (ø)
pkg/cmd/render/render.go 50.00% <66.66%> (+2.75%) ⬆️

... and 5 files with indirect coverage changes

pkg/cmd/render/render.go Outdated Show resolved Hide resolved
pkg/cmd/render/render_test.go Outdated Show resolved Hide resolved
@qJkee qJkee changed the title [WIP] OCPVE-647: annotate manifests with capability name OCPVE-647: annotate manifests with capability name Nov 23, 2023
@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 Nov 23, 2023
@qJkee
Copy link
Contributor Author

qJkee commented Nov 23, 2023

/hold until openshift/api#1571 gets merged

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 28, 2023

@qJkee: This pull request references OCPVE-647 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

add CloudCredential capability name to all manifests to make component optional

Also, i had to modify the render function. Now, if capability is disabled, we do render nothing

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.

@dlom
Copy link
Contributor

dlom commented Nov 28, 2023

/retest

@dlom
Copy link
Contributor

dlom commented Nov 28, 2023

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 28, 2023
go.mod Outdated Show resolved Hide resolved
@jstuever
Copy link
Contributor

Depends on openshift/api#1571

@qJkee qJkee force-pushed the OCPVE-647 branch 3 times, most recently from 86a1337 to fb43d39 Compare December 4, 2023 20:02
add CloudCredential capability name to all manifests to make
component optional
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
@fxierh
Copy link
Contributor

fxierh commented Dec 5, 2023

Why do we annotate the manifests under ./bindata/bootstrap ?
AFAIK those are to be rendered and applied by cluster-bootstrap which does not respect CVO annotations like "[capability.openshift.io/name](http://capability.openshift.io/name): CloudCredential".

@qJkee
Copy link
Contributor Author

qJkee commented Dec 5, 2023

@fxierh
I'd like to keep this manifests annotated just in case if someone would use them somewhere else

@fxierh
Copy link
Contributor

fxierh commented Dec 5, 2023

I'd like to keep this manifests annotated just in case if someone would use them somewhere else

This makes me wonder why we maintain two sets of manifests, i.e. the ./bindata/bootstrap ones, and the ./manifests ones.

modify render function to return nothing
in case if CCO is disabled in the cluster
Bump priority of the manifests in CVO order.
This change is needed because some operators had
higher priority for credentials request than
actual CRD. This change solving the issue.
@qJkee
Copy link
Contributor Author

qJkee commented Dec 5, 2023

/retest

@qJkee
Copy link
Contributor Author

qJkee commented Dec 5, 2023

I'd like to keep this manifests annotated just in case if someone would use them somewhere else

This makes me wonder why we maintain two sets of manifests, i.e. the ./bindata/bootstrap ones, and the ./manifests ones.

Well, it's because of bindata
I think it's worth moving to go embed instead of copying them from vendor dir =)

@qJkee
Copy link
Contributor Author

qJkee commented Dec 6, 2023

/retest

@dlom
Copy link
Contributor

dlom commented Dec 6, 2023

/test e2e-upgrade

@dlom
Copy link
Contributor

dlom commented Dec 6, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlom, qJkee

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

@qJkee
Copy link
Contributor Author

qJkee commented Dec 7, 2023

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2023
Copy link
Contributor

openshift-ci bot commented Dec 7, 2023

@qJkee: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit ba0e0a8 into openshift:master Dec 7, 2023
10 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cloud-credential-operator-container-v4.16.0-202312071150.p0.gba0e0a8.assembly.stream for distgit ose-cloud-credential-operator.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants