Skip to content

🌱 Update catalogd e2e and update e2e - #1222

Closed
tmshort wants to merge 1 commit into
operator-framework:mainfrom
tmshort:update-catd-e2e
Closed

🌱 Update catalogd e2e and update e2e#1222
tmshort wants to merge 1 commit into
operator-framework:mainfrom
tmshort:update-catd-e2e

Conversation

@tmshort

@tmshorttmshort commented Sep 5, 2024

Copy link
Copy Markdown
Contributor

Fixes: #1115

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@tmshort
tmshort requested a review from a team as a code ownerSeptember 5, 2024 19:08
@netlify

netlifyBot commented Sep 5, 2024•
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commitb17f1c1
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66df38c30c432600088c897f
😎 Deploy Previewhttps://deploy-preview-1222--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tmshort

tmshort commented Sep 5, 2024

Copy link
Copy Markdown
ContributorAuthor

The upgrade-e2e failure is likely, due to the addition of a webhook

@codecov

codecovBot commented Sep 5, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.53%. Comparing base (f8c9077) to head (b17f1c1).

Additional details and impacted files
@@ Coverage Diff @@## main #1222 +/- ##
=======================================
Coverage 76.53% 76.53% =======================================
Files 40 40 Lines 2340 2340 =======================================
Hits 1791 1791 Misses 392 392 Partials 157 157
FlagCoverage Δ
e2e57.64% <ø> (ø)
unit52.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joelanford
joelanford reviewed Sep 5, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
}
}

func TestClusterCatalogLabels(t *testing.T) {

@joelanfordjoelanfordSep 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this separate test in operator-controller? It seems like

  1. it is functionality that is fully encompassed (and now tested) in catalogd.
  2. we are already testing this indirectly via operator-controller's support of catalog selectors in its ClusterExtension API.

@tmshorttmshortSep 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If catalogd's behavior changes, then this would detect it, but so would other tests. I can live without it

@joelanfordjoelanfordSep 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but so would other tests

Right, our other tests depend on that functionality working now.

@everettraveneverettravenSep 5, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 on not testing catalogd's behavior explicitly in operator-controller

@tmshort

tmshort commented Sep 6, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

It would seem that using GenerateName, as used here:

operator-controller/test/extension-developer-e2e/extension_developer_test.go

Line 44 in c4470cc

GenerateName: "catalog",
won't work with ClusterCatalogs...

@perdasilva

perdasilva commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2024
perdasilva
perdasilva previously approved these changes Sep 6, 2024
View reviewed changes
@everettraven

everettraven commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

It would seem that using GenerateName, as used here:

operator-controller/test/extension-developer-e2e/extension_developer_test.go

Line 44 in c4470cc

GenerateName: "catalog",

won't work with ClusterCatalogs...

Why is that?

@tmshort

tmshort commented Sep 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Why is that?

Because the name field doesn't exist, the webhook matchConditions errors out. This has been fixed in my latest merged PR to catalogd: operator-framework/catalogd#382

@tmshort
Update to catalogd v0.26.0 and update e2e tests
b17f1c1
* Use proper labels from catalogd
* Add better deployment checking
* Fix broken test
This test seems broken, it's supposed to resolve again, and the
test implies that this should be the 2.0.0 version, but it's
checking against 1.2.0 (which was the original version installed)
Signed-off-by: Todd Short <tshort@redhat.com>
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2024
@openshift-ci

openshift-ciBot commented Sep 9, 2024

Copy link
Copy Markdown

New changes are detected. LGTM label has been removed.

@tmshorttmshort closed this Sep 9, 2024
@joelanfordjoelanford mentioned this pull request Sep 9, 2024
Merged
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanfordjoelanfordjoelanford left review comments
@perdasilvaperdasilvaperdasilva left review comments
+1 more reviewer
@everettraveneverettraveneverettraven left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@perdasilvaperdasilva

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

E2E tests for labeling webhooks and selector

4 participants

@tmshort@perdasilva@everettraven@joelanford