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

Resource conversion, one solo-kit.json per project #214

Closed
wants to merge 18 commits into from

Conversation

joekelley
Copy link
Contributor

@joekelley joekelley commented Jul 16, 2019

WIP: Need to do some more cleanup and validation.

This PR adds support for converting resources from one version to another, and redefines a "project" from the de facto "one solo-kit.json file per version" to the preferred "one solo-kit.json file per set of versioned api groups". The latter is a breaking change.
BOT NOTES:
resolves #215

pkg/code-generator/cmd/main.go Outdated Show resolved Hide resolved
pkg/code-generator/cmd/main.go Outdated Show resolved Hide resolved
pkg/code-generator/cmd/main.go Show resolved Hide resolved
pkg/code-generator/codegen/conversion.go Outdated Show resolved Hide resolved
pkg/code-generator/codegen/conversion.go Outdated Show resolved Hide resolved
pkg/code-generator/model/project.go Outdated Show resolved Hide resolved
pkg/code-generator/model/project.go Show resolved Hide resolved
pkg/code-generator/parser/parser_resource.go Show resolved Hide resolved
test/mocks/conversion/resource_converter.sk.go Outdated Show resolved Hide resolved
@solo-git-bot
Copy link

solo-git-bot bot commented Jul 16, 2019

Issues linked to changelog:
#215

if err != nil {
return err
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

why are we doing this copy? there's a resources.Clone function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The copy needs to happen in place onto dst -- clone returns a new object

if err != nil {
return err
}
err = protoutils.UnmarshalBytes(srcBytes, dst)
Copy link
Member

Choose a reason for hiding this comment

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

nit:

if err := protoutils.UnmarshalBytes(srcBytes, dst); err != nil {
	return err
}

return true
for _, skp := range soloKitProjects {
for _, vc := range skp.ApiGroup.VersionConfigs {
if strings.HasPrefix(protoFile, filepath.Dir(skp.ProjectFile)+"/"+vc.Version) {
Copy link
Member

Choose a reason for hiding this comment

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

this means solo-kit.json is now expected to live (always) in the parent directory of the version dirs? if so, it might be useful to provide a log line warning or error if the user somehow misses this point

for _, vc := range skp.ApiGroup.VersionConfigs {
vc.CustomResources = append(vc.CustomResources, importedResources...)
for _, desc := range descriptors {
if filepath.Dir(desc.ProtoFilePath) == filepath.Dir(skp.ProjectFile)+"/"+vc.Version {
Copy link
Member

Choose a reason for hiding this comment

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

seems like we have this check in a few places. can we extract to a function?
also, use filepath.Join here. even though it's only required on windows. and if you use windows, you're wrong 😏

@joekelley joekelley closed this Dec 19, 2019
@sam-heilbron sam-heilbron deleted the conversion-one-sk branch December 11, 2021 01:40
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.

Generate a converter for multi-version resources
3 participants