Bug 1829994: Index generate dameonless #314
Conversation
openshift-ci-robot
added
the
bugzilla/severity-urgent
|
@kevinrizza: This pull request references Bugzilla bug 1829994, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
openshift-ci-robot
added
the
approved
|
/retest |
cmd/opm/index/add.go
Outdated
| indexCmd.Flags().StringP("tag", "t", "", "custom tag for container image being built") | ||
| indexCmd.Flags().Bool("permissive", false, "allow registry load errors") | ||
| indexCmd.Flags().StringP("mode", "", "replaces", "graph update mode that defines how channel graphs are updated. One of: [replaces, semver, semver-skippatch]") | ||
| indexCmd.Flags().String("container-tool", "", "container-tool has been deprecated. Please select a build and pull tool separately.") |
There was a problem hiding this comment.
Note for later: cobra has a way to mark options as deprecated:
indexCmd.Flags().StringP("container-tool", "c", "podman", "tool to interact with container images (save, build, etc.). One of: [docker, podman]")
if err := rootCmd.Flags().MarkDeprecated("container-tool", "ignored in favor of separate build-tool and pull-tool options"); err != nil {
logrus.Panic(err.Error())
}This has some nice properties like preserving the help when referenced, but omitting it from the standard help output.
There was a problem hiding this comment.
Ah, nice. I'll update this to include it.
cmd/opm/index/add.go
Outdated
| return err | ||
| } | ||
|
|
||
| if containerTool != "" { |
There was a problem hiding this comment.
Any thoughts on how backwards compatible we want to be? If we explode here we end up breaking existing clients. Though we can do a major version bump after to be dogmatic about semver.
There was a problem hiding this comment.
I was sort of back and forth on this. I think that's fair, maybe we want to just treat this as an alias and use it to overwrite defaults if the two new flags haven't been specified?
There was a problem hiding this comment.
SGTM, but is there a convention for telling the difference between unset and set-to-default?
kevinrizza
force-pushed
the
index-generate-dameonless
branch
from
8d3dc77
to
6581031
Compare
|
/lgtm |
openshift-ci-robot
added
the
do-not-merge/hold
|
/lgtm |
|
/retest |
kevinrizza
force-pushed
the
index-generate-dameonless
branch
from
6581031
to
4a975e2
Compare
|
@ecordell updated based on your thoughts. ptal. |
Multiple runs of index add when --generate is specified because the db file cannot be cleaned up. To account for this, update the copyDatabaseTo function to not return when the parent folder already exists and overwrite the actual database when doing io.Copy()
Enable daemonless pull by default on indexes. When no build happens, this makes opm index add daemonless
kevinrizza
force-pushed
the
index-generate-dameonless
branch
from
4a975e2
to
d3c0563
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, gallettilance, kevinrizza 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 |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/test e2e-aws |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@kevinrizza: All pull requests linked via external trackers have merged: operator-framework/operator-registry#314. Bugzilla bug 1829994 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Enable daemonless index add when
--generateis specified.This also fixes a bug where, on multiple runs with --generate specified, the database would fail to build.
Doing this with a partial implementation to unblock podman users to be able to generate the database without shelling out to podman and hitting this bug containers/podman#5234. In the future, this implementation should be fleshed out so that all of the registry and index commands make this distinction between pull and build runners.