Skip to content

Conversation

asmacdo
Copy link
Member

@asmacdo asmacdo commented Jun 9, 2022

Passing off to @everettraven since I'll be out today. I will be on slack if there are any questions.

If the tests don't pass we will make this a feature branch, but for the sake of go.mod merge conflicts, lets try to avoid that if possible. IMO an admin-merge could be appropriate if the tests will be fixed quickly.

Signed-off-by: Austin Macdonald austin@redhat.com

Closes: #5833

Signed-off-by: Austin Macdonald <austin@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven everettraven added the release-blocker This issue blocks the parent release milestone label Jun 9, 2022
@everettraven everettraven added this to the v1.22.0 milestone Jun 9, 2022
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Cool 🥇

Some nits;

a) we need to fix the changelog and ensure that it has only what impacts the end-users
also, changes in the scaffolds need migration steps

b) I do not think that we need/should change the Leader election here. We need to check how that will affect endusers, can we do a PR only for that?

@everettraven
Copy link
Contributor

everettraven commented Jun 9, 2022

@camilamacedo86

a) we need to fix the changelog and ensure that it has only what impacts the end-users also, changes in the scaffolds need migration steps

I'll make some changes to the changelog to reflect this.

b) I do not think that we need/should change the Leader election here. We need to check how that will affect endusers, can we do a PR only for that?

Unfortunately I don't think we can do it in a separate PR as client-go/leaderelection/resourcelock that comes with v0.24.0 forces the change. My understanding is that this change should be a safe step in the migration to leases and only affect new operators and not older operators. I am open to discussing this further, but this needs to be a part of this PR.

As an extra note, our e2e tests have passed with the changes as well.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022
@everettraven
Copy link
Contributor

/hold

still a WIP

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2022
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Copy link
Contributor

@camilamacedo86 camilamacedo86 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2022
@everettraven
Copy link
Contributor

@camilamacedo86 Thanks so much for all the help with the changelog!

@everettraven
Copy link
Contributor

/hold

We need to bump the OPM version in the Makefile scaffolds to be 1.23.0

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2022
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2022
@asmacdo
Copy link
Member Author

asmacdo commented Jun 10, 2022

Also fixes #5854

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven everettraven linked an issue Jun 10, 2022 that may be closed by this pull request
Copy link
Member Author

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

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

some nits, dont hold bc of this review

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven everettraven removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2022
Copy link
Member Author

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

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

LGTM (I cant approve because I am the OP)

the sanity/docs job is failing due to 1 broken link: https://v1-15-x.sdk.operatorframework.io/ which does work. This is 100% just a flake, I feel comfortable doing an admin merge to keep this moving.

@camilamacedo86
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2022
@camilamacedo86
Copy link
Contributor

/approved

Great work !!!

@everettraven everettraven merged commit 025df62 into operator-framework:master Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. release-blocker This issue blocks the parent release milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump golang 1.17 -> 1.18
5 participants