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

Better naming for controllers deployment #278

Merged
merged 2 commits into from
Apr 16, 2019
Merged

Better naming for controllers deployment #278

merged 2 commits into from
Apr 16, 2019

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Apr 10, 2019

Better naming for controllers deployment

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 10, 2019
@enxebre enxebre changed the title Naming Better naming for controllers deployment Apr 10, 2019
michaelgugino
michaelgugino approved these changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2019
michaelgugino
michaelgugino suggested changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm cancel

owned-manifests/machine-api-controllers.yaml
image: {{ .Controllers.Provider }}
command:
- "./manager"
- /manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, to align with all other command and other fields syntax

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2019
michaelgugino
michaelgugino approved these changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2019
@enxebre
Copy link
Member Author

enxebre commented Apr 10, 2019

/test integration

@enxebre
Copy link
Member Author

enxebre commented Apr 10, 2019

/retest

@frobware
Copy link
Contributor

/lgtm

@enxebre
Copy link
Member Author

enxebre commented Apr 11, 2019

/restest

@enxebre
Copy link
Member Author

enxebre commented Apr 11, 2019

/retest

bison
bison reviewed Apr 11, 2019
View reviewed changes
vendor/github.com/openshift/cluster-api-actuator-pkg/pkg/e2e/operators/machine-api-operator.go
@@ -23,7 +23,7 @@ var _ = g.Describe("[Feature:Operators] Machine API operator deployment should",
client, err := e2e.LoadClient()
o.Expect(err).NotTo(o.HaveOccurred())

deploymentName := "clusterapi-manager-controllers"
deploymentName := "machine-api-controllers"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a PR for this on cluster-api-actuator-pkg?

Copy link
Member Author

Choose a reason for hiding this comment

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

openshift/cluster-api-actuator-pkg#63

@enxebre
Copy link
Member Author

enxebre commented Apr 11, 2019

/test e2e-aws-operator

@enxebre
Copy link
Member Author

enxebre commented Apr 11, 2019

we are hitting https://bugzilla.redhat.com/show_bug.cgi?id=1698624 re-triggering for CI measurement

@cynepco3hahue
Copy link
Contributor

/retest

@spangenberg
Copy link
Contributor

/lgtm

@enxebre
Copy link
Member Author

enxebre commented Apr 11, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2019
@ingvagabund
Copy link
Member

This PR is no longer relevant. We have already a PR that moves the yaml manifest under the code:
#280

ingvagabund
ingvagabund requested changes Apr 14, 2019
View reviewed changes
owned-manifests/machine-api-controllers.yaml Outdated Show resolved Hide resolved
owned-manifests/machine-api-controllers.yaml Outdated Show resolved Hide resolved
owned-manifests/machine-api-controllers.yaml Outdated Show resolved Hide resolved
@ingvagabund
Copy link
Member

Unless, there is an issue fixed by this PR, let's wait for #280 to merge and then rebase.

@enxebre enxebre mentioned this pull request Apr 15, 2019
Merged
@enxebre enxebre force-pushed the naming branch 3 times, most recently from f94817e to 557a3a6 Compare April 15, 2019 14:39
@enxebre
Copy link
Member Author

enxebre commented Apr 15, 2019

this should go green now #286
/retest

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 15, 2019
@enxebre
Copy link
Member Author

enxebre commented Apr 15, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 15, 2019
@enxebre
Copy link
Member Author

enxebre commented Apr 15, 2019 

level=warning msg="Found override for ReleaseImage. Please be warned, this is not advised"
level=info msg="Consuming \"Install Config\" from target directory"
level=info msg="Creating infrastructure resources..."
level=error
level=error msg="Error: Error applying plan:"
level=error
level=error msg="1 error occurred:"
level=error msg="\t* module.vpc.aws_route.igw_route: 1 error occurred:"
level=error msg="\t* aws_route.igw_route: Error creating route: timeout while waiting for state to become 'success' (timeout: 2m0s)"
level=error
level=error
level=error
level=error

/retest

@ingvagabund
Copy link
Member

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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 Apr 15, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 15, 2019 •
edited
Loading

@enxebre: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/actuator-pkg-staleness 80fd312 link /test actuator-pkg-staleness

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@ingvagabund
Copy link
Member

/test e2e-aws

@frobware
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2019
@openshift-merge-robot openshift-merge-robot merged commit 1c4f553 into openshift:master Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ingvagabund ingvagabund ingvagabund requested changes

@frobware frobware frobware left review comments

@bison bison bison left review comments

@michaelgugino michaelgugino michaelgugino approved these changes

@spangenberg spangenberg Awaiting requested review from spangenberg

Assignees

@spangenberg spangenberg

@frobware frobware

@michaelgugino michaelgugino

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@enxebre @frobware @cynepco3hahue @spangenberg @ingvagabund @openshift-ci-robot @bison @michaelgugino @openshift-merge-robot