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

Move to Uber mock/mockgen fork #3863

Merged
merged 7 commits into from
Sep 26, 2024

Conversation

kimorris27
Copy link
Contributor

@kimorris27 kimorris27 commented Sep 24, 2024

Which issue this PR addresses:

https://issues.redhat.com/browse/ARO-4360

What this PR does / why we need it:

This PR removes our dependency on https://github.com/golang/mock and completely switches us over to using https://github.com/uber-go/mock instead for two reasons:

  1. It needs to be done eventually anyway
  2. It happens to fix an issue that I ran into generating mocks for some new Azure clients I added in a MIWI PR (the golang version doesn't seem to be up-to-date enough to handle generics properly in certain contexts)

Additionally, I upgrade our go-toolset container image from 1.21.11 to 1.21.13 to satisfy Uber mockgen's dependency on Go >= 1.21.12. The corresponding ADO PR is here: https://msazure.visualstudio.com/AzureRedHatOpenShift/_git/ARO.Build.Images/pullrequest/10921698

Test plan for issue:

CI and E2E

Is there any documentation that needs to be updated for this PR?

I don't think so, but let me know if you know of any.

How do you know this will function as expected in production?

E2E during the next release :D

@edisonLcardenas
Copy link
Collaborator

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@bitoku
Copy link
Collaborator

bitoku commented Sep 25, 2024

It seems like we don't have go-toolset:1.21.13-1.

Dockerfile.aro-e2e:4
--------------------
   2 |     #
   3 |     ARG REGISTRY
   4 | >>> FROM ${REGISTRY}/ubi8/go-toolset:1.21.13-1 AS builder
   5 |     
   6 |     USER root
--------------------
ERROR: failed to solve: arointsvc.azurecr.io/ubi8/go-toolset:1.21.13-1: failed to resolve source metadata for arointsvc.azurecr.io/ubi8/go-toolset:1.21.13-1: arointsvc.azurecr.io/ubi8/go-toolset:1.21.13-1: not found
make: *** [Makefile:183: image-e2e] Error 1

@tsatam
Copy link
Collaborator

tsatam commented Sep 25, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam
Copy link
Collaborator

tsatam commented Sep 25, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Collaborator

@shubhadapaithankar shubhadapaithankar left a comment

Choose a reason for hiding this comment

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

LGTM

Uber fork, then `go mod tidy` and `go mod vendor`

Note the change to `deps.go`
…tes when we were using both repos at the same time
Mocks for these interfaces were previously present, but if you remove them and make generate, they don't get replaced. I'm guessing that when they were added, the committer forgot to commit their changes to the generate.go files. This came to my attention as I was moving us over to the Uber fork because it caused errors while I was trying to get builds and unit tests working, so I codified the generation properly in this commit.
@kimorris27 kimorris27 force-pushed the kimorris27/ARO-4360-move-to-uber-mock-fork branch from e5c6afd to 7448b3d Compare September 26, 2024 14:00
@kimorris27
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@hlipsig hlipsig left a comment

Choose a reason for hiding this comment

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

Non-blocking question.

@@ -1,5 +1,5 @@
module _ // Auto generated by https://github.com/bwplotka/bingo. DO NOT EDIT

go 1.21
go 1.21.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Goutham is trying to upgrade us to go 1.22. I see they've added support for 22 recently. Any reason to merget this at 1.21 and then update to 1.22?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in Slack. We decided to merge this PR as-is since it's blocking stuff, and I'll branch off of Goutham's 1.22 working branch to prepare another 1.22 PR related to this stuff as needed.

@hlipsig
Copy link
Contributor

hlipsig commented Sep 26, 2024

Based on a quick conversation with Kipp to prevent merge conflicts or other strangeness we're going to merge this as is with version 1.21 of golang, then create a follow on PR using the branch from #3797 to ensure everything here works with 1.22. Since this blocks other critical work this feels like the best route.

@hlipsig hlipsig merged commit e985b50 into master Sep 26, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants