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

Migration source and target implementations #124

Merged
merged 3 commits into from
Dec 14, 2022

Conversation

sergenyalcin
Copy link
Member

Description of your changes

Depends on #119
Related https://github.com/upbound/team-extensions/issues/29

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested as part of upbound/team-control-planes#119

type FileSystemSourceOption func(*FileSystemSource)

// FsWithFileSystem configures the filesystem to use. Used mostly for testing.
func FsWithFileSystem(f afero.Fs) FileSystemSourceOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any fields other than the Fs that we might configure (so that we may prefer to use an afero.Afero as a parameter instead of an afero.Fs in the functional option)?

pkg/migration/filesystem.go Outdated Show resolved Hide resolved
pkg/migration/filesystem.go Outdated Show resolved Hide resolved
pkg/migration/filesystem.go Outdated Show resolved Hide resolved
pkg/migration/filesystem.go Outdated Show resolved Hide resolved
// we need to add plural appendix to end of kind name
Resource: strings.ToLower(gvk.Kind) + "s",
})
unstructuredList, err := ri.List(context.TODO(), metav1.ListOptions{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

We had better have an active context here. We should consider revising the Source & Target interfaces.

pkg/migration/kubernetes.go Outdated Show resolved Hide resolved
pkg/migration/kubernetes.go Outdated Show resolved Hide resolved
pkg/migration/kubernetes.go Outdated Show resolved Hide resolved
}

// InitializeDynamicClient returns a dynamic client
func InitializeDynamicClient(kubeconfigPath string) (dynamic.Interface, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We consider this as a utility to construct dynamic clients, is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is correct!

@Upbound-CLA
Copy link

Upbound-CLA commented Nov 23, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin for the implementations. I think we are ready to merge them and do some further iterations as we gain more experience on the migration use cases and corner cases.

Because you are busy with support rotation, I can take care of rebasing the branch and merging it as is. Also the PR #119 that contains the base commit of this PR's feature branch has been merged.

@sergenyalcin sergenyalcin merged commit 15411ad into crossplane:main Dec 14, 2022
@sergenyalcin sergenyalcin deleted the source-target branch December 14, 2022 12:37
@ulucinar
Copy link
Collaborator

Hi @sergenyalcin,
I have rebased your PR. I only did the following changes in your feature branch:

  • Moved test resources awsvpc.yaml and resourcegroup.yaml from testdata to testdata/source.
  • Added the following error check in the test file pkg/migration/filesystem_test.go:
			fs, err := NewFileSystemSource("testdata/source", FsWithFileSystem(files))
			if err != nil {
				t.Fatalf("Failed to initialize a new FileSystemSource: %v", err)
			}
  • Updated references to testdata with testdata/source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants