Skip to content

Conversation

cynepco3hahue
Copy link

@cynepco3hahue cynepco3hahue commented Jan 7, 2019

  • Align k8s.io dependencies version to k8s-1.11.2
  • Get rid of kubernetes-incubator/apiserver-builder dependency
    This package depends on release-1.9 or on release-1.12, in both
    cases it does not work with our dependencies

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 7, 2019
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 7, 2019
@openshift-ci-robot
Copy link
Contributor

Hi @cynepco3hahue. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Gopkg.toml Outdated
name = "k8s.io/code-generator"
# revision for tag "kubernetes-1.11.2"
revision = "6702109cc68eb6fe6350b83e14407c8d7309fd1a"
version = "kubernetes-1.11.1"
Copy link
Member

Choose a reason for hiding this comment

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

Why not 1.11.2?

Copy link
Author

Choose a reason for hiding this comment

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

I checked other openshift repositories that we depend on like github.com/openshift/api, github.com/openshift/client-go and they use kubernetes-1.11.1

Copy link
Member

Choose a reason for hiding this comment

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

fair enough. Hopefully there is no signification difference (e.g. known errors in 1.11.1 fixed in 1.11.2).

Copy link
Author

Choose a reason for hiding this comment

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

@ingvagabund, in the end, I updated dependencies to 1.11.2 because controller-runtime depends on it

// NewTestConfig creates new test config with clients
func NewTestConfig(kubeconfig string) *TestConfig {
config, err := controller.GetConfig(kubeconfig)
config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
Copy link
Member

Choose a reason for hiding this comment

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

+1

sigs.k8s.io/cluster-api-machineset: worker
spec:
providerConfig:
providerSpec:
Copy link
Member

@ingvagabund ingvagabund Jan 7, 2019

Choose a reason for hiding this comment

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

Ha, this got forgotten. Please, remove this from the PR. The field can be renamed once we remove the .providerConfig field completely in upcoming PR.

@ingvagabund
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 7, 2019
@cynepco3hahue cynepco3hahue force-pushed the update_vendor_dependencies branch 2 times, most recently from 9a08cfc to 0c6a834 Compare January 8, 2019 12:07
@cynepco3hahue
Copy link
Author

/retest

@cynepco3hahue cynepco3hahue force-pushed the update_vendor_dependencies branch from 0c6a834 to dd6e72c Compare January 8, 2019 16:33
@cynepco3hahue
Copy link
Author

/retest

@cynepco3hahue
Copy link
Author

@ingvagabund hi, do you know how can I update actuator images with the latest changes?
I can see that https://hub.docker.com/r/openshift/origin-aws-machine-controllers update 19 days ago, so it definitely does not include providerSpec changes.

@ingvagabund
Copy link
Member

@cynepco3hahue yeah, the job responsible for building the docker images is most likely broken/not running. @paulfantom can you take a look at it?

@ingvagabund
Copy link
Member

@cynepco3hahue can you break the commit into two? One that updates deps and one that changes the mao code? It's easier to review then. Plus, the PR summary does not reflect what the PR does. Can you more elaborate on that? E.g. Remove kubernetes-incubator/apiserver-builder dependency

@cynepco3hahue
Copy link
Author

@ingvagabund I separated change where I drop kubernetes-incubator/apiserver-builder dependency under separate PR #169 and will rebase this one after it.

@paulfantom
Copy link
Contributor

@cynepco3hahue we have images on quay, for this repo image is stored in https://quay.io/repository/openshift/origin-machine-api-operator

@cynepco3hahue
Copy link
Author

@paulfantom Do I need to update this repo to take images from quay and from docker?

@paulfantom
Copy link
Contributor

Yes, we should use quay where possible.

@cynepco3hahue
Copy link
Author

@paulfantom Can you point me to related images under the quay.io? I just tried to search for machine-api-operator and cluster-api-provider-aws and failed to find it.

@cynepco3hahue
Copy link
Author

@paulfantom Thanks will update images to relevant one

@cynepco3hahue cynepco3hahue force-pushed the update_vendor_dependencies branch 2 times, most recently from b6a6d61 to 79f1a9d Compare January 10, 2019 13:57
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2019
@ingvagabund
Copy link
Member

@cynepco3hahue can you rebase?

@cynepco3hahue
Copy link
Author

@ingvagabund Sure and sorry for the delay, I had some snowboard PTO 😄

@ingvagabund
Copy link
Member

@ingvagabund Sure and sorry for the delay, I had some snowboard PTO 😄

Take your time :) Not urgent, relaxing and having good time is better :P

@cynepco3hahue cynepco3hahue force-pushed the update_vendor_dependencies branch from 79f1a9d to 848e66a Compare January 27, 2019 12:37
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2019
@cynepco3hahue cynepco3hahue force-pushed the update_vendor_dependencies branch 2 times, most recently from fc99d85 to 33ab181 Compare January 27, 2019 14:12
@cynepco3hahue
Copy link
Author

@ingvagabund Rebased, can you please review again?

revision = "1624edc4454b8682399def8740d46db5e4362ba4"
version = "v1.1.5"

[[projects]]
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this? How did it end up in the lock file?

Copy link
Author

Choose a reason for hiding this comment

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

Hm, I just ran dep ensure -v, I will check the dependency tree to see from where it is coming.

Copy link
Author

@cynepco3hahue cynepco3hahue Jan 28, 2019

Choose a reason for hiding this comment

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

@enxebre Ok, logrus v1.3.0 has this dependency, I see only test/integration has the dependency on the logrus, can we drop it in favor of glog?

Copy link
Member

Choose a reason for hiding this comment

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

definitely sg

Name: optr.name,
},
Status: osconfigv1.ClusterOperatorStatus{
Version: version.Raw,
Copy link
Member

Choose a reason for hiding this comment

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

This belongs to a different commit. This effort it's being tracked here https://jira.coreos.com/browse/CLOUD-332

Copy link
Author

@cynepco3hahue cynepco3hahue Jan 28, 2019

Choose a reason for hiding this comment

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

I need to update it here because of dep updates to the latest changes, otherwise, I will have a compilation error, at least from CI I can see that it does not break anything.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2019
- align k8s.io dependencies version to k8s-1.11.2
- use exact version 0.1.7 for `controller-runtime` package
@cynepco3hahue cynepco3hahue force-pushed the update_vendor_dependencies branch from 33ab181 to f843367 Compare February 17, 2019 13:02
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: enxebre

If they are not already assigned, you can assign the PR to them by writing /assign @enxebre in a comment when ready.

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

@cynepco3hahue
Copy link
Author

/retest

@cynepco3hahue
Copy link
Author

After the pivoting of cluster API this PR does not relevant anymore.

ingvagabund pushed a commit to ingvagabund/machine-api-operator that referenced this pull request Jul 11, 2019
Drop cluster test since it's not relevant for the actuator
germanparente pushed a commit to germanparente/machine-api-operator that referenced this pull request Sep 23, 2025
Bug 1948719: update controller-runtime dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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