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

use common repo functionality in SNR (Node Role labels) #131

Merged
merged 2 commits into from
Jul 5, 2023
Merged

use common repo functionality in SNR (Node Role labels) #131

merged 2 commits into from
Jul 5, 2023

Conversation

mshitrit
Copy link
Member

@mshitrit mshitrit commented Jul 2, 2023

No description provided.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 2, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved label Jul 2, 2023
@mshitrit mshitrit changed the title use common repo functionality in SNR (Node Role labels) [WIP] use common repo functionality in SNR (Node Role labels) Jul 2, 2023
@mshitrit
Copy link
Member Author

mshitrit commented Jul 2, 2023

/test ?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 2, 2023

@mshitrit: The following commands are available to trigger required jobs:

  • /test 4.11-ci-index-self-node-remediation-bundle
  • /test 4.11-images
  • /test 4.11-openshift-e2e
  • /test 4.11-test
  • /test 4.12-ci-index-self-node-remediation-bundle
  • /test 4.12-images
  • /test 4.12-openshift-e2e
  • /test 4.12-test
  • /test 4.13-ci-index-self-node-remediation-bundle
  • /test 4.13-images
  • /test 4.13-openshift-e2e
  • /test 4.13-test
  • /test 4.14-ci-index-self-node-remediation-bundle
  • /test 4.14-images
  • /test 4.14-openshift-e2e
  • /test 4.14-test

Use /test all to run all jobs.

In response to this:

/test ?

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.

@mshitrit
Copy link
Member Author

mshitrit commented Jul 2, 2023

/test 4.13-openshift-e2e

@mshitrit
Copy link
Member Author

mshitrit commented Jul 2, 2023 •
edited
Loading

Hmm IIUC the problem here is that on the one hand common (which is consumed by SNR in this PR) requires k8s.io/apimachinery v0.26.3 instead of k8s.io/apimachinery v0.25.0 which was previously used in SNR, and on the other hand SNR consumes github.com/openshift/machine-api-operator which in turn consumes the older version of k8s.io/apimachinery (and does not support the newer version).
AFAIS there is no newer version of github.com/openshift/machine-api-operator which supports k8s.io/apimachinery v0.26.3+.

So IIUC in theory the following options are:

  • see what needs to be done in order to get a newer version of github.com/openshift/machine-api-operator (which supports k8s.io/apimachinery v0.26.3+)
  • downgrade k8s.io/apimachinery in common
  • close this PR (i.e SNR will not consume common)
  • remove usage of github.com/openshift/machine-api-operator from SNR

@mshitrit mshitrit mentioned this pull request Jul 2, 2023
Merged
@mshitrit
Copy link
Member Author

mshitrit commented Jul 3, 2023

/test 4.13-openshift-e2e

@mshitrit mshitrit force-pushed the use_common branch 2 times, most recently from 87e4925 to 183b9a8 Compare July 3, 2023 09:13
@mshitrit
Copy link
Member Author

mshitrit commented Jul 3, 2023

/test 4.13-openshift-e2e

@mshitrit mshitrit changed the title [WIP] use common repo functionality in SNR (Node Role labels) use common repo functionality in SNR (Node Role labels) Jul 3, 2023
@razo7
Copy link
Member

razo7 commented Jul 3, 2023

/lgtm
Giving others a chance to review as wel, feel free to unhold
/hold

@razo7 razo7 marked this pull request as ready for review July 3, 2023 12:27
@openshift-ci openshift-ci bot requested review from clobrano and slintes July 3, 2023 12:27
clobrano
clobrano reviewed Jul 3, 2023
View reviewed changes
pkg/render/render.go
@@ -24,7 +22,6 @@ type Data struct {
}

type Config struct {
*mcfgv1.ControllerConfigSpec
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why this was removed

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a miss on my part, has to do with some experiments I've tried and forgot to revert.
good catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

However looking at the change I'm not sure it's wrong in that context.
It eliminates the dependency in machine-config-operator and it looks like this config isn't used.

slintes
slintes requested changes Jul 3, 2023
View reviewed changes
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

see comment inside

@openshift-ci openshift-ci bot removed the lgtm label Jul 3, 2023
slintes
slintes reviewed Jul 3, 2023
View reviewed changes
main.go Outdated Show resolved Hide resolved
@mshitrit
Copy link
Member Author

mshitrit commented Jul 5, 2023

/retest

1 similar comment
@mshitrit
Copy link
Member Author

mshitrit commented Jul 5, 2023

/retest

@openshift-ci openshift-ci bot added the lgtm label Jul 5, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mshitrit, slintes

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

@slintes
Copy link
Member

slintes commented Jul 5, 2023

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 8bae0fe into medik8s:main Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clobrano clobrano clobrano left review comments

@slintes slintes slintes approved these changes

Assignees

@slintes slintes

@razo7 razo7

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mshitrit @razo7 @slintes @clobrano @openshift-merge-robot