Add support for Project Descriptor 0.2

[YousefHaggyHeroku]

Summary

Implementing this without breaking the old descriptor parsing. I'm not sure how to approach documentation with a change like this

Output

Before

After

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #1127

[YousefHaggyHeroku]

I wonder if there's a better name for this 🤷 , maybe just common? thoughts?

[aemengo]

No strong feeling over here!

[aemengo]

This looks great! You're pretty good at this 🙂

--

Also wanted to the highlight that the following windows CI errors aren't related to this PR.

# ...
Setting up auth
  "C:\Program Files\Git\cmd\git.exe" config --local --name-only --get-regexp core\.sshCommand
  "C:\Program Files\Git\cmd\git.exe" submodule foreach --recursive "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
  Error: fatal: No url found for submodule path 'internal/registry/registry139553254' in .gitmodules
  Error: The process 'C:\Program Files\Git\cmd\git.exe' failed with exit code 128

[aemengo]

No strong feeling over here!

[aemengo]

These are internal only representations right? Meaning these won't be Marshalled(), Unmarshalled() to the filesystem?

If so, then these toml:"..." annotations won't be used.

[YousefHaggyHeroku]

These are meant to be primarily internal representations. However, currently v01/package.go and v02/package.go reference these structs for Unmarshalling some of the unchanged attributes (ex: v02 uses common.Buildpack).

I'm not sure whether we should just copy over the structs and remove struct tags from common.go or just leave as is, I'm comfortable with either.

[jromero]

I'm comfortable straddling both sides and treating common primarily as the internal representations with the (un)marshalling until there is a deviation between different versions.

[aemengo]

I'm personally finding the self-updating structs a little awkward.

Suggested change

	func ReadProjectDescriptor(pathToFile string) (common.Descriptor, error) {
	func ReadProjectDescriptor(pathToFile string) (common.Descriptor, error) {
	pathToFile = filepath.Clean(pathToFile)
	var versionDescriptor struct {
	Project struct {
	Version string `toml:"schema-version"`
	} `toml:"_"`
	}
	_, err := toml.DecodeFile(pathToFile, &versionDescriptor)
	if err != nil {
	return common.Descriptor{}, errors.Wrapf(err, "parsing schema version")
	}
	switch versionDescriptor.Project.Version {
	// _.schema-version does not exist in 0.1
	case "0.1", "":
	descriptor, err := v01.NewDescriptor(pathToFile)
	if err != nil {
	return common.Descriptor{}, err
	}
	return descriptor, validate(descriptor)
	case "0.2":
	descriptor, err := v02.NewDescriptor(pathToFile)
	if err != nil {
	return common.Descriptor{}, err
	}
	return descriptor, validate(descriptor)
	default:
	return common.Descriptor{}, fmt.Errorf("unknown project descriptor schema version %s", versionDescriptor.Project.Version)
	}
	}

[YousefHaggyHeroku]

I like this suggestion a lot! I think it looks a lot cleaner and less awkward. The only possible downside I could see is if we have lots of versions in the future, the switch statement could get a little large and redundant. What are your thoughts on the following:

var NewDescriptor = map[string]common.NewDescriptorType{
	"0.1": v01.NewDescriptor,
	"0.2": v02.NewDescriptor,
}

func ReadProjectDescriptor(pathToFile string) (common.Descriptor, error) {
	projectTomlContents, err := ioutil.ReadFile(filepath.Clean(pathToFile))
	if err != nil {
		return common.Descriptor{}, err
	}

	var versionDescriptor struct {
		Project struct {
			Version string `toml:"schema-version"`
		} `toml:"_"`
	}

	_, err = toml.Decode(string(projectTomlContents), &versionDescriptor)
	if err != nil {
		return common.Descriptor{}, errors.Wrapf(err, "parsing schema version")
	}

	version := versionDescriptor.Project.Version
	if version == "" {
		version = "0.1"
	}

	var descriptor common.Descriptor

	if _, ok := NewDescriptor[version]; !ok {
		return common.Descriptor{}, fmt.Errorf("unknown project descriptor schema version %s", version)
	}

	descriptor, err = NewDescriptor[version](projectTomlContents)
	if err != nil {
		return common.Descriptor{}, err
	}

	return descriptor, validate(descriptor)

}

I'm also totally happy to land on your suggestion; I do think it's the cleanest solution for the present. No strong feelings here, just wanted to put out one more thought 😄

[aemengo]

I don't know why I didn't get a notification. I'm so sorry for missing this! 🙇🏾

Ultimately, I think it's a style preference which is why I have no problem with what you've presented (both of your suggestions). Your code is plenty clear as it is. In my personal projects, large and redundant switch statements are very common; But I understand that it's not for everyone!

[jromero]

Since we slacked a bit here @YousefHaggyHeroku I'm going through the process of creating a polish PR for this so we can get this in ASAP. I liked and therefore am taking your latest suggestion into the follow up PR. I'll also take care of the merge conflicts for you.

[YousefHaggy]

Sounds good! Thanks for making that PR