Skip to content

[run bundle] refactor catalog source and registry pod creation#3832

Merged
rashmigottipati merged 1 commit intooperator-framework:masterfrom
rashmigottipati:fix-for-catalogsource
Sep 8, 2020
Merged

[run bundle] refactor catalog source and registry pod creation#3832
rashmigottipati merged 1 commit intooperator-framework:masterfrom
rashmigottipati:fix-for-catalogsource

Conversation

@rashmigottipati
Copy link
Copy Markdown
Member

@rashmigottipati rashmigottipati commented Sep 2, 2020 •
edited
Loading

  • Few minor fixes to get catalog source create, wait and update working correctly for run bundle subcommand
  • Tested these changes manually by running the command and it works correctly
  • Leaving watiForCatalogSource() function commented out for now

jmrodri
jmrodri approved these changes Sep 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

I just have a question, otherwise LGTM

Comment thread internal/olm/operator/registry/index/registry_pod.go Outdated
@jmrodri
Copy link
Copy Markdown
Member

jmrodri commented Sep 2, 2020

@rashmigottipati looks like your changes broke this test: TestOLMIntegration/PackageManifestsMultiplePackages

joelanford
joelanford approved these changes Sep 2, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Looks great overall! Just some nits.

Otherwise,
/lgtm

Comment thread internal/olm/operator/registry/index_image.go Outdated
Comment thread internal/olm/operator/registry/index_image.go Outdated
Comment thread internal/olm/operator/registry/operator_installer.go Outdated
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020
joelanford
joelanford reviewed Sep 2, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Oh, we can also remove the log statements here and here whenever. I put those there as placeholders with my boilerplate PR.

@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Sep 2, 2020

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020
estroz
estroz suggested changes Sep 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@estroz estroz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the issue with OperatorInstaller.waitForCatalogSource() was resolved, so it should be left commented here.

Comment thread internal/olm/operator/registry/operator_installer.go
@jmrodri jmrodri mentioned this pull request Sep 3, 2020
Closed
14 tasks
@rashmigottipati rashmigottipati changed the title [run bundle] fix catalog source create and update [run bundle] refactor catalog source and registry pod creation Sep 3, 2020
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/operator_installer.go
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index_image.go
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index/registry_pod.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index/registry_pod.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index_image.go
@joelanford joelanford self-requested a review September 4, 2020 14:01
@rashmigottipati rashmigottipati force-pushed the fix-for-catalogsource branch 2 times, most recently from 7825702 to 3cb7091 Compare September 4, 2020 17:12
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index/registry_pod.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index/registry_pod.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index_image.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index_image.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index_image.go Outdated
estroz
estroz reviewed Sep 4, 2020
View reviewed changes
Comment thread internal/olm/operator/registry/index_image.go Outdated
estroz
estroz suggested changes Sep 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Mostly small nits, otherwise looks good.

@rashmigottipati rashmigottipati force-pushed the fix-for-catalogsource branch 2 times, most recently from 38cc1e5 to 472828c Compare September 8, 2020 21:52
estroz
estroz approved these changes Sep 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

One correction then LGTM

Comment thread internal/olm/operator/registry/index/registry_pod.go
// and throws error if unable to parse and execute the container command
func (rp *RegistryPod) getContainerCmd() (string, error) {
const containerCommand = "/bin/mkdir -p {{ .DBPath }} &&" +
const containerCommand = "/bin/mkdir -p {{ .DBPath | dirname }} &&" +
Copy link
Copy Markdown
Member

@estroz estroz Sep 8, 2020

Choose a reason for hiding this comment

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

NB: this may cause a similar issue in OCP described here.

Comment thread internal/olm/operator/registry/index_image.go Outdated
@rashmigottipati rashmigottipati merged commit aed6d38 into operator-framework:master Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmrodri jmrodri jmrodri approved these changes

@bharathi-tenneti bharathi-tenneti Awaiting requested review from bharathi-tenneti

@joelanford joelanford Awaiting requested review from joelanford

+1 more reviewer

@estroz estroz estroz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@joelanford joelanford

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rashmigottipati @jmrodri @openshift-ci-robot @joelanford @estroz