Skip to content

Add composite veneer to opm alpha render-veneer ... #1054

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 8 commits into from
Jan 20, 2023
Merged

Add composite veneer to opm alpha render-veneer ... #1054

merged 8 commits into from
Jan 20, 2023

Conversation

@everettraven
Copy link
Contributor

@everettraven everettraven commented Jan 17, 2023

Description of the change:

  • Adds new subcommand opm alpha render-veneer composite [FILE] [OPTIONS]
  • Adds a new composite package that adds:
    • A new composite veneer type
    • Builders for building different veneer types in the composite configuration
    • Schema for catalog configuration
    • Schema for catalog component configuration
    • Unit tests

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

Note for Reviewers:

  • There are quite a few TODO comments sprinkled around with some questions - please feel free to include your inputs on these questions as part of your review!

@everettraven
initial implementation pass
7eb8d6b 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
initial implementation pass
70a394a 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
update implementation
f374183 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
add unit tests
6f7b72b 
and any changes necessary in relation to adding unit tests.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot requested review from anik120 and hasbro17 January 17, 2023 20:39
@everettraven
Copy link
Contributor Author

everettraven commented Jan 17, 2023

/cc @grokspawn @joelanford

@codecov
Copy link

codecov bot commented Jan 17, 2023 •
edited
Loading

Codecov Report

Merging #1054 (a9e6f68) into master (0080ea0) will increase coverage by 0.81%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
+ Coverage   52.06%   52.87%   +0.81%     
==========================================
  Files         102      104       +2     
  Lines        9218     9403     +185     
==========================================
+ Hits         4799     4972     +173     
- Misses       3505     3515      +10     
- Partials      914      916       +2
Impacted Files Coverage Δ
alpha/declcfg/load.go 76.52% <70.37%> (-2.13%) ⬇️
alpha/veneer/composite/builder.go 97.20% <97.20%> (ø)
alpha/veneer/composite/composite.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

grokspawn
grokspawn reviewed Jan 18, 2023
View reviewed changes
@everettraven
verify schemas and fail if they don't match
606acd5 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
add better validations and decrease some code duplication
f33598f 
as well as add the necessary additional test cases

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
grokspawn
grokspawn reviewed Jan 19, 2023
View reviewed changes
grokspawn
grokspawn reviewed Jan 19, 2023
View reviewed changes
grokspawn
grokspawn reviewed Jan 19, 2023
View reviewed changes
grokspawn
grokspawn reviewed Jan 19, 2023
View reviewed changes
grokspawn
grokspawn reviewed Jan 19, 2023
View reviewed changes
@everettraven
remove some TODO comments and hardcode containerTool as 'docker'
bfde6b3 
as per review comments

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
update the custom veneer builder
a9e6f68 
to work like the other veneer builders by reading a full FBC from STDOUT
and writing it to the output destination in the custom veneer config.
Also adds additional test cases to cover the new changes.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2023
gallettilance
gallettilance approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@gallettilance gallettilance left a comment

Choose a reason for hiding this comment

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

/lgtm - just one comment maybe for a follow up.

cmd/opm/alpha/veneer/composite.go
cmd.Flags().BoolVar(&validate, "validate", true, "whether or not the created FBC should be validated (i.e 'opm validate')")
cmd.Flags().StringVarP(&compositeFile, "composite-config", "c", "catalog/config.yaml", "File to use as the composite configuration file")
cmd.Flags().StringVarP(&catalogFile, "catalog-config", "f", "catalogs.yaml", "File to use as the catalog configuration file")
return cmd
Copy link
Member

@gallettilance gallettilance Jan 20, 2023

Choose a reason for hiding this comment

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

could we add a flag to account for a previous catalog generation so we dont have to pull all images again? or an overwrite option for the output in case it's non empty so we can warn users if they make a veneer change that would strand users on a particular version for example?

Copy link
Contributor

@grokspawn grokspawn Jan 20, 2023

Choose a reason for hiding this comment

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

Are you thinking about the use-case where we want to make incremental updates only to an existing catalog? According to the document (https://docs.google.com/document/d/1uEZdVA06GGahPl3ygnGrFLRkrohaQjs25mVcXToYakE/edit#heading=h.kjc5gjiy2pkd) SOP will be to overwrite the specific contribution in all cases.

I'd be interested in talking more about the use-case, but def not barring this PR merge.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, gallettilance, grokspawn

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

@gallettilance
Copy link
Member

gallettilance commented Jan 20, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@openshift-merge-robot openshift-merge-robot merged commit 104e027 into operator-framework:master Jan 20, 2023
@grokspawn grokspawn mentioned this pull request Mar 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grokspawn grokspawn grokspawn approved these changes

@anik120 anik120 Awaiting requested review from anik120

@hasbro17 hasbro17 Awaiting requested review from hasbro17

@joelanford joelanford Awaiting requested review from joelanford

+1 more reviewer

@gallettilance gallettilance gallettilance approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gallettilance gallettilance

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.

6 participants

@everettraven @gallettilance @joelanford @anik120 @grokspawn @openshift-merge-robot