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

(vendor) k8s + catalogd bump #475

Merged
merged 2 commits into from
Nov 3, 2023
Merged

(vendor) k8s + catalogd bump #475

merged 2 commits into from
Nov 3, 2023

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Oct 20, 2023

Description

Reviewer Checklist

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

@anik120 anik120 requested a review from a team as a code owner October 20, 2023 16:00
everettraven
everettraven previously approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Aside from the lint warning this looks good to me

@m1kola
Copy link
Member

m1kola commented Oct 23, 2023 •
edited
Loading

Heads up, I think the operator-developer-e2e failure is legit, but not related to this PR. I think it is related to #444, but I'm not 100% sure (tests were green on #444 as well as some other PRs which came after it).

I'm investigating.

Edit: It looks like some of the failures across our PRs were due to golang/go#63684 and I no longer see issues in other PRs. So it might be related to this PR after all?

@anik120 anik120 force-pushed the bump-k8s-catalogd branch 2 times, most recently from 1b801f5 to efa84d8 Compare October 23, 2023 20:20
This was referenced Oct 24, 2023
Closed
Closed
Closed
joelanford
joelanford previously approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Needs a go.mod conflict resolution, but looks good to me otherwise.

@m1kola
Copy link
Member

m1kola commented Oct 24, 2023

Edit: It looks like some of the failures across our PRs were due to golang/go#63684 and I no longer see issues in other PRs. So it might be related to this PR after all?

Correction: operator-developer-e2e failures are not related to this PR after all (see #481 and PR for this - #488)

m1kola
m1kola reviewed Oct 24, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@anik120 anik120 force-pushed the bump-k8s-catalogd branch 6 times, most recently from 6dc5483 to 645f15f Compare October 25, 2023 18:08
@codecov
Copy link

codecov bot commented Oct 25, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9081f96) 83.69% compared to head (70bf85c) 83.69%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #475   +/-   ##
=======================================
  Coverage   83.69%   83.69%           
=======================================
  Files          23       23           
  Lines         877      877           
=======================================
  Hits          734      734           
  Misses         97       97           
  Partials       46       46
Flag Coverage Δ
e2e 65.22% <100.00%> (ø)
unit 78.68% <0.00%> (ø)

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

Files Coverage Δ
internal/controllers/operator_controller.go 80.00% <100.00%> (ø)

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

@anik120 anik120 force-pushed the bump-k8s-catalogd branch 3 times, most recently from af4dfb7 to cf58cf7 Compare October 27, 2023 15:50
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 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 Oct 27, 2023
This was referenced Oct 27, 2023
Closed
Closed
@everettraven everettraven mentioned this pull request Oct 31, 2023
Merged
4 tasks
@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 1, 2023
anik120 and others added 2 commits November 2, 2023 14:11
@anik120 @everettraven
(vendor) k8s + catalogd bump
9e3857a 
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>

stand up registry for e2e

Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
@everettraven
refactor test utilities to be individual configurable components
70bf85c 
and update the operator-framework-e2e test to use an
on-cluster image registry and build+push the test
catalog to it

Signed-off-by: Bryce Palmer <everettraven@gmail.com>
@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 2, 2023
@grokspawn grokspawn added this pull request to the merge queue Nov 3, 2023
m1kola
m1kola reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

I went through about half of it. So far noticed only a minor thing.

test/operator-framework-e2e/setup.sh
@@ -18,31 +18,35 @@ The following environment variables are required for configuring this script:
setup.sh also takes 5 arguments.

Usage:
setup.sh [OPERATOR_SDK] [CONTAINER_RUNTIME] [KUSTOMIZE] [KIND] [KIND_CLUSTER_NAME]
setup.sh [OPERATOR_SDK] [CONTAINER_RUNTIME] [KUSTOMIZE] [KIND] [KIND_CLUSTER_NAME] [NAMESPACE]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like help text above needs to be updated.

Also - do we want to have a check for non-empty namespace? Similar to CATALOG_IMG, REG_PKG_NAME, etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, does look like I forgot to increase the argument count there. I think it could be helpful to check for a non-empty namespace but I think requiring it to be an argument alleviates the need for that. You'd have to explicitly make it an empty value

Merged via the queue into operator-framework:main with commit aca6451 Nov 3, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m1kola m1kola m1kola left review comments

@everettraven everettraven everettraven left review comments

@grokspawn grokspawn grokspawn approved these changes

@joelanford joelanford joelanford left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@anik120 @m1kola @joelanford @everettraven @grokspawn @openshift-merge-robot