-
Notifications
You must be signed in to change notification settings - Fork 1
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
added cli bootstrap journey acceptance test #3527
Conversation
pkg/bootstrap/utils/k8s.go
Outdated
|
||
err = schemeBuilder.AddToScheme(scheme) | ||
//TODO use kube package as has also utils to create clients | ||
scheme, err := kube.CreateScheme() |
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.
@waleedhammam the kube
package is the one in oss for managing kube clients, it is definitely useful to leverage as much as we can from it in the context of #3507
@@ -7,7 +7,7 @@ import ( | |||
"github.com/alecthomas/assert" | |||
helmv2 "github.com/fluxcd/helm-controller/api/v2beta1" | |||
"github.com/fluxcd/pkg/apis/meta" | |||
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" | |||
sourcev1beta2 "github.com/fluxcd/source-controller/api/v1beta2" |
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.
shall we use v1
for all, seems we're using v1
and v1beta2
?
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.
it depends on the resource on flux ga: gitrepositories is v1 and helmrepo are v1beta2 we should use the right one by resource.
pkg/bootstrap/utils/testutils/k8s.go
Outdated
) | ||
|
||
// CreateKubeconfigFileForRestConfig creates a kubeconfig file so we could use it for calling any command with --kubeconfig pointing to it | ||
func CreateKubeconfigFileForRestConfig(restConfig rest.Config) (string, error) { |
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.
I think we could move this file to test utils package
as we did with create fake client ?
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.
actually i think the other way around: is using /test
packages something best practice? i know i reviewed and approved, but i might have not realised on the path
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.
I think since they're generic methods it could useful somewhere else, noting so specific about bootstrapping IMO
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.
👍 changed there
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.
LGTM
Closes #3508
What changed?
Why was this change made?
How was this change implemented?
How did you validate the change?
or, explain why there are no new tests
there tests that fail without the change?
Release notes
No
Documentation Changes
Added internal
Other follow ups