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

feat(registry): Add label dependency constraint and property #406

Merged
merged 2 commits into from
Jul 31, 2020
Merged

feat(registry): Add label dependency constraint and property #406

merged 2 commits into from
Jul 31, 2020
+510 −31

Conversation

dinhxuanvu
Copy link
Member

  1. Introduce a new type of dependency constraint named olm.label which
    represents the constraints that are based on CSV labels.
  2. The new label property is parsed from CSV labels.

Signed-off-by: Vu Dinh [email protected]

Description of the change:

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

Sorry, something went wrong.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020
@dinhxuanvu
Copy link
Member Author

/hold until PR is reviewed and lgtm-ed.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020
@dinhxuanvu dinhxuanvu added the area/dependency Issues or PRs related to dependency changes label Jul 28, 2020
kevinrizza
kevinrizza requested changes Jul 28, 2020
View reviewed changes
Copy link
Member

@kevinrizza kevinrizza left a comment

Choose a reason for hiding this comment

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

Just some minor comments about tests. Otherwise lgtm

@dinhxuanvu
Copy link
Member Author

/retest

benluddy
benluddy reviewed Jul 28, 2020
View reviewed changes
@dinhxuanvu
Copy link
Member Author

I wonder if this e2e-aws failure is valid or not. The error message doesn't seem to be OLM-related.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2020
@dinhxuanvu
feat(registry): Add label dependency constraint and property
eb48f76 
1. Introduce a new type of dependency constraint named `olm.label` which
represents the constraints that are based on CSV labels.
2. The new label property is parsed from CSV labels.

Signed-off-by: Vu Dinh <[email protected]>
@dinhxuanvu
Update label property extraction from CSV
2e30704 
The label properties are now specified in CSV annotations.

Signed-off-by: Vu Dinh <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2020
@dinhxuanvu
Copy link
Member Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020
@dinhxuanvu
Copy link
Member Author

/retest

@kevinrizza
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

anik120
anik120 reviewed Jul 30, 2020
View reviewed changes
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

Couple of non-blocking nits. Otherwise lgtm.

pkg/sqlite/load.go
if csv, err := bundle.ClusterServiceVersion(); err == nil {
annotations := csv.ObjectMeta.GetAnnotations()
if v, ok := annotations[registry.PropertyKey]; ok {
var props []registry.Property
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
var props []registry.Property
var properties []registry.Property

pkg/sqlite/load.go
if v, ok := annotations[registry.PropertyKey]; ok {
var props []registry.Property
if err := json.Unmarshal([]byte(v), &props); err == nil {
for _, prop := range props {
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
for _, prop := range props {
for _, property := range properties {

docs/design/operator-bundle.md

An example of a `dependencies.yaml` that specifies Prometheus operator and etcd CRD dependencies:

```
dependencies:
- type: olm.package
value:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Noise.

@anik120
Copy link
Contributor

anik120 commented Jul 30, 2020

Looks like it's been lgtm-ed, if approved please resolve my comments they're very minor.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@dinhxuanvu
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 19378f2 into operator-framework:master Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benluddy benluddy

@anik120 anik120

@kevinrizza kevinrizza

@ecordell ecordell

@Bowenislandsong Bowenislandsong

Assignees

@kevinrizza kevinrizza

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes 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.

None yet
7 participants
@dinhxuanvu @openshift-ci-robot @kevinrizza @openshift-bot @anik120 @benluddy @openshift-merge-robot