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.

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

Add 'Paused' to ImageChangeTrigger for BuildConfig #19624

Closed

Conversation

wozniakjan
Copy link
Contributor

@wozniakjan wozniakjan commented May 4, 2018

When user adds ICT to BC, the image_trigger_controller.go starts a build and this may not be desired for certain scenarios where specifically only change in ImageStreamTag should trigger the build. Setting Paused: true tells the controller to not trigger the build.

https://trello.com/c/iT4bw5jg/1541-3-ability-to-set-imagechange-triggers-in-buildconfig-without-triggering-a-build-automatically

cc: @openshift/sig-developer-experience, @openshift/sig-master

NOTE: This also changes the default behavior, which I think is probably not desired. For example, when user oc create -f [bc.yaml] with ICT without Automatic field set, a build won't be triggered now. Maybe we would like to set Automatic field by default to true?

TODO:

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/master labels May 4, 2018
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 4, 2018
@wozniakjan
Copy link
Contributor Author

@bparees
Copy link
Contributor

bparees commented May 4, 2018

Maybe we would like to set Automatic field by default to true?

yes we need to add a defaulter to default this field to true, or change the name to something like disabled. (I like automatic as a name since it matches what the DC does, though).

we'll have to sort that out along w/ the general shape of the api in your api PR.

@bparees
Copy link
Contributor

bparees commented May 4, 2018

pending resolution of the api discussion, this looks reasonable to me (would need tests of course, but i wouldn't start writing those until the api is settled)

@bparees
Copy link
Contributor

bparees commented May 4, 2018

also one question on the desired behavior of the api to be resolved here:
openshift/api#44 (comment)

@wozniakjan wozniakjan force-pushed the feature/imagechangetrigger branch 2 times, most recently from 62639da to 8a07d6f Compare May 7, 2018 14:53
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2018
@wozniakjan wozniakjan changed the title [WIP] Add 'Automatic' to ImageChangeTrigger for BuildConfig [WIP] Add 'Paused' to ImageChangeTrigger for BuildConfig May 7, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2018
@bparees bparees self-assigned this May 17, 2018
@bparees bparees added this to the v3.11 milestone May 17, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018
@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 25, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, wozniakjan

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 Jun 25, 2018
@bparees
Copy link
Contributor

bparees commented Jun 25, 2018

/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 Jun 25, 2018
@openshift-bot
Copy link
Contributor

/retest

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

3 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.

@wozniakjan
Copy link
Contributor Author

/retest

flake PROVISION CLOUD RESOURCES

@wozniakjan
Copy link
Contributor Author

/retest

flake PROVISION CLOUD RESOURCES

1 similar comment
@wozniakjan
Copy link
Contributor Author

/retest

flake PROVISION CLOUD RESOURCES

@bparees
Copy link
Contributor

bparees commented Jun 26, 2018

/retest

@wozniakjan
Copy link
Contributor Author

/retest

1 similar comment
@wozniakjan
Copy link
Contributor Author

/retest

@wozniakjan
Copy link
Contributor Author

/retest

clusterup - Failed to install "openshift-web-console-operator"
conformance - PROVISION CLOUD RESOURCES
gcp - {"component":"entrypoint","level":"error","msg":"Process did not finish before 2h0m0s timeout","time":"2018-06-27T08:29:36Z"}

@php-coder
Copy link
Contributor

/retest

@wozniakjan
Copy link
Contributor Author

wozniakjan commented Jun 27, 2018

@stevekuznetsov the GCP job keeps timing out here. I don't think this change could introduce such a regression that tests will start timing out, any ideas how to debug/fix?

the job doesn't seem to exist in jenkins https://ci.openshift.redhat.com/jenkins/job/pull-ci-origin-launch-gcp/14/ (or maybe just the url is invalid?)

2018/06/27 13:58:51 Container setup in pod e2e-gcp completed successfully
{"component":"entrypoint","level":"error","msg":"Process did not finish before 2h0m0s timeout","time":"2018-06-27T15:09:35Z"}
2018/06/27 15:09:35 error: Process interrupted with signal interrupt, exiting in 2s ...
2018/06/27 15:09:35 cleanup: Deleting template e2e-gcp
{"component":"entrypoint","level":"error","msg":"Process gracefully exited before 15s grace period","time":"2018-06-27T15:09:37Z"}

@php-coder
Copy link
Contributor

/retest

@bparees
Copy link
Contributor

bparees commented Jun 28, 2018

@wozniakjan @php-coder @simo5 per some discussion w/ @deads2k we need to coordinate the two PRs that are picking up api changes, into a single PR. Specifically there is a new commit in api that needs to be picked up for anything to merge, so we need a single PR that picks up 1) the paused api change 2) @php-coder's api change and 3) @deads2k bug fix api change.

@php-coder as i recall you were going to create your PR rebased on top of this PR, did you do that? If so, can you point us to the PR and is it ready for merge? It will need to be re-glide-up'd and rebased.

If it's not ready for merge, we may need to consider another path to get this PR in.

@php-coder
Copy link
Contributor

@php-coder as i recall you were going to create your PR rebased on top of this PR, did you do that? If so, can you point us to the PR and is it ready for merge? It will need to be re-glide-up'd and rebased.

@bparees Here is my PR -- #18398

I've just tried to update it but couldn't make it work as make update fails to me now (see output with errors below).

@bparees @wozniakjan @deads2k Feel free to cherry-pick my commit(s) to use them in your PRs.

hack/build-go.sh  
++ Building go targets for linux/amd64: cmd/hypershift cmd/openshift cmd/oc cmd/oadm cmd/template-service-broker cmd/openshift-node-config vendor/k8s.io/kubernetes/cmd/hyperkube pkg/network/sdn-cni-plugin vendor/github.com/containernetworking/plugins/plugins/ipam/host-local vendor/github.com/containernetworking/plugins/plugins/main/loopback
# github.com/openshift/origin/pkg/cmd/infra/router
pkg/cmd/infra/router/clientcmd.go:69:46: undefined: genericclioptions.OpenShiftKubeConfigFlagName
pkg/cmd/infra/router/clientcmd.go:70:32: undefined: genericclioptions.OpenShiftKubeConfigFlagName
# github.com/openshift/origin/pkg/oc/admin/policy
pkg/oc/admin/policy/cani.go:85:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/cani.go:138:18: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrinterForOptions
pkg/oc/admin/policy/cani.go:138:45: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".ExtractCmdPrintOptions
pkg/oc/admin/policy/modify_roles.go:96:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:135:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:171:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:209:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:238:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:270:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:300:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/admin/policy/modify_roles.go:300:2: too many errors
# github.com/openshift/origin/pkg/oc/admin/groups/sync/cli
pkg/oc/admin/groups/sync/cli/sync.go:180:2: undefined: util.AddPrinterFlags
pkg/oc/admin/groups/sync/cli/sync.go:432:35: undefined: util.PrintObject
# github.com/openshift/origin/pkg/oc/admin/migrate
pkg/oc/admin/migrate/migrator.go:151:2: undefined: util.AddNonDeprecatedPrinterFlags
pkg/oc/admin/migrate/migrator.go:161:19: undefined: util.PrinterForOptions
pkg/oc/admin/migrate/migrator.go:161:46: undefined: util.ExtractCmdPrintOptions
# github.com/openshift/origin/pkg/oc/cli/util/clientcmd
pkg/oc/cli/util/clientcmd/helpers.go:19:10: undefined: util.ExtractCmdPrintOptions
pkg/oc/cli/util/clientcmd/helpers.go:20:18: undefined: util.PrinterForOptions
# github.com/openshift/origin/pkg/oc/admin/node
pkg/oc/admin/node/listpods.go:26:2: undefined: util.AddPrinterFlags
pkg/oc/admin/node/node_options.go:69:21: undefined: util.PrinterForOptions
pkg/oc/admin/node/node_options.go:69:48: undefined: util.ExtractCmdPrintOptions
pkg/oc/admin/node/node_options.go:92:10: undefined: util.PrinterForOptions
pkg/oc/admin/node/node_options.go:92:37: undefined: util.ExtractCmdPrintOptions
# github.com/openshift/origin/pkg/oc/cli/config
pkg/oc/cli/config/loader.go:12:62: undefined: genericclioptions.OpenShiftKubeConfigFlagName
pkg/oc/cli/config/loader.go:20:21: undefined: genericclioptions.OpenShiftKubeConfigFlagName
# github.com/openshift/origin/pkg/oc/cli/cmd/create
pkg/oc/cli/cmd/create/clusterquota.go:67:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/cli/cmd/create/clusterquota.go:134:10: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintObject
pkg/oc/cli/cmd/create/clusterquota.go:169:3: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintSuccess
pkg/oc/cli/cmd/create/deploymentconfig.go:65:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/cli/cmd/create/deploymentconfig.go:121:10: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintObject
pkg/oc/cli/cmd/create/deploymentconfig.go:157:3: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintSuccess
pkg/oc/cli/cmd/create/identity.go:68:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/cli/cmd/create/identity.go:102:10: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintObject
pkg/oc/cli/cmd/create/identity.go:144:3: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintSuccess
pkg/oc/cli/cmd/create/imagestream.go:75:2: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".AddPrinterFlags
pkg/oc/cli/cmd/create/imagestream.go:75:2: too many errors
# github.com/openshift/origin/pkg/oc/admin/registry
pkg/oc/admin/registry/registry.go:495:36: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintObject
# github.com/openshift/origin/pkg/oc/admin/router
pkg/oc/admin/router/router.go:834:36: undefined: "github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util".PrintObject
# github.com/openshift/origin/pkg/oc/experimental/ipfailover
pkg/oc/experimental/ipfailover/ipfailover.go:221:37: undefined: util.PrintObject
# github.com/openshift/origin/pkg/oc/cli/cmd/importer
pkg/oc/cli/cmd/importer/appjson.go:146:45: undefined: util.PrintObject
# github.com/openshift/origin/pkg/oc/cli/secrets
pkg/oc/cli/secrets/basicauth.go:87:23: undefined: util.PrintObject
pkg/oc/cli/secrets/basicauth.go:105:2: undefined: util.AddPrinterFlags
pkg/oc/cli/secrets/dockercfg.go:87:23: undefined: util.PrintObject
pkg/oc/cli/secrets/dockercfg.go:102:2: undefined: util.AddPrinterFlags
pkg/oc/cli/secrets/new.go:97:23: undefined: util.PrintObject
pkg/oc/cli/secrets/new.go:109:2: undefined: util.AddPrinterFlags
pkg/oc/cli/secrets/sshauth.go:81:23: undefined: util.PrintObject
pkg/oc/cli/secrets/sshauth.go:98:2: undefined: util.AddPrinterFlags
# github.com/openshift/origin/pkg/oc/experimental/config
pkg/oc/experimental/config/patch.go:69:2: undefined: util.AddPrinterFlags
pkg/oc/experimental/config/patch.go:91:15: undefined: util.ExtractCmdPrintOptions
pkg/oc/experimental/config/patch.go:94:19: undefined: printers.GetStandardPrinter

@openshift-bot
Copy link
Contributor

@wozniakjan: PR needs rebase.

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-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2018
@openshift-ci-robot
Copy link

@wozniakjan: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/integration 30d5e4b link /test integration
ci/openshift-jenkins/cmd 56891d2 link /test cmd
ci/openshift-jenkins/end_to_end 56891d2 link /test end_to_end
ci/openshift-jenkins/extended_conformance_install 56891d2 link /test extended_conformance_install
ci/openshift-jenkins/extended_builds 56891d2 link /test extended_builds
ci/openshift-jenkins/extended_clusterup 56891d2 link /test extended_clusterup
ci/openshift-jenkins/extended_image_ecosystem 56891d2 link /test extended_image_ecosystem
ci/openshift-jenkins/cross 56891d2 link /test cross
ci/openshift-jenkins/extended_image_registry 56891d2 link /test extended_image_registry
ci/prow/integration 56891d2 link /test integration
ci/prow/verify 56891d2 link /test verify
ci/prow/gcp 56891d2 link /test gcp
ci/prow/unit 56891d2 link /test unit

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. I understand the commands that are listed here.

@bparees
Copy link
Contributor

bparees commented Jun 29, 2018

this is being handled in #20152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/build lgtm Indicates that a PR is ready to be merged. needs-api-review needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/developer-experience sig/master size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants