-
Notifications
You must be signed in to change notification settings - Fork 253
update gomock #127
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
update gomock #127
Conversation
dgoodwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny nit, otherwise looks good, I was going to point out you can just update specific deps but it sounds like you did, so dep bites us again. Thanks for taking on and solving this.
pkg/install/convertconfig_test.go
Outdated
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| ic, err := GenerateInstallConfig(test.cd, adminPassword, adminSSHKey, pullSecret) | ||
| ic, err := GenerateInstallConfig(test.cd /* adminPassword, */, adminSSHKey, pullSecret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
no longer need admin username/password update all the places that used clusterDeployment.spec.config.admin
|
/lgtm |
| // The two types are extremely similar, but have different goals and in some cases deviation was required | ||
| // as ClusterDeployment is used as a CRD API. | ||
| // | ||
| // It is assumed the caller will lookup the admin password and ssh key from their respective secrets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "admin password" reference is stale now. I'm not sure what to replace it with though, because I don't understand the whole "caller will lookup" idea. The caller is passing these into GenerateInstallConfig, so why would they need to look them up at all? And how do secrets come into this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In hive's ClusterDeployment the sshKey and pullSecret (and previously admin password) are referenced via secrets. Whatever code calls this needs to look them up, I didn't want this function responsible for talking to etcd.
PRs failing like so https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_hive/120/pull-ci-openshift-hive-master-unit/208
Update gomock/mockgen