Skip to content

add velero deployment to CSV so velero SA/roles are added to CSV then removing it #482

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
Merged
merged 5 commits into from
Dec 3, 2021
Merged

add velero deployment to CSV so velero SA/roles are added to CSV then removing it #482

merged 5 commits into from
Dec 3, 2021

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented Nov 30, 2021 •
edited
Loading

This fixes make bundle not adding velero roles/SA to CSV.

Velero placeholder deployment is added to config so operator-sdk generate bundle add cluster permission rules for velero SA in CSV.
yq is used to remove placeholder deployment from final bundle CSV.

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021 •
edited
Loading

Codecov Report

Merging #482 (ca0ae84) into master (c1c6b2d) will not change coverage.
The diff coverage is n/a.

❗ Current head ca0ae84 differs from pull request most recent head bb754de. Consider uploading reports for the commit bb754de to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #482   +/-   ##
=======================================
  Coverage   37.51%   37.51%           
=======================================
  Files          12       12           
  Lines        2287     2287           
=======================================
  Hits          858      858           
  Misses       1358     1358           
  Partials       71       71

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0adb97b...bb754de. Read the comment docs.

@kaovilai kaovilai marked this pull request as draft December 1, 2021 16:42
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 1, 2021
@kaovilai kaovilai force-pushed the makebundleveleroroleincsv branch from c64ea02 to 9589474 Compare December 1, 2021 21:35
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@kaovilai kaovilai force-pushed the makebundleveleroroleincsv branch from bb754de to 6dc9005 Compare December 1, 2021 21:42
@kaovilai kaovilai marked this pull request as ready for review December 1, 2021 21:42
@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 Dec 1, 2021
@kaovilai kaovilai requested review from djzager and dymurray December 1, 2021 21:45
@kaovilai kaovilai force-pushed the makebundleveleroroleincsv branch from 6395ed0 to 3c038e9 Compare December 1, 2021 21:52
@kaovilai kaovilai changed the title add velero deployment to CSV so velero SA/roles are added to CSV add velero deployment to CSV so velero SA/roles are added to CSV then removing it Dec 1, 2021
@kaovilai
Copy link
Member Author

kaovilai commented Dec 1, 2021

/test operator-e2e

1 similar comment
@kaovilai
Copy link
Member Author

kaovilai commented Dec 2, 2021

/test operator-e2e

djzager
djzager approved these changes Dec 2, 2021
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

I personally prefer this to manually maintaining the csv. Have an ack.

config/velero/velero.yaml
@@ -0,0 +1,120 @@
# This is a placeholder deployment config so operator-sdk add cluster-roles for velero to ClusterServiceVersion
# It will be removed by yq during make bundle
kind: Deployment
Copy link
Member

Choose a reason for hiding this comment

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

My only recommendation for this file is to strip it to make it less something that needs to be maintained.

As I understand it, the only thing that matters is that the deployment's serviceAccountName.

Copy link
Member Author

Choose a reason for hiding this comment

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

@djzager just pulled this from a running development deployment.. nothing much to maintain. Just need a container to be a valid deployment

@dymurray
Copy link
Member

dymurray commented Dec 3, 2021

/retest

1 similar comment
@dymurray
Copy link
Member

dymurray commented Dec 3, 2021

/retest

@dymurray
Copy link
Member

dymurray commented Dec 3, 2021

This is definitely a weird way to solve things, but it gets the job done for now. We are going to want to revisit this at some point

@dymurray dymurray merged commit dc2e703 into openshift:master Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwmatthews jwmatthews Awaiting requested review from jwmatthews

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

@dymurray dymurray Awaiting requested review from dymurray

1 more reviewer

@djzager djzager djzager approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kaovilai @codecov-commenter @dymurray @djzager