Make starter Chart.yaml the source of truth for all versions

[miles-w-3]

No description provided.

[oraNod]

Thanks a lot for this @miles-w-3 it seems like a really good approach. I've got some minor comments for now but will come back to the get_app_version function with a suggestion in a couple of hours. I think I see one or two things that might be an improvement. Cheers.

[oraNod]

Do we want to go ahead and change the chart version to CalVer or should we do that as a follow up?

[miles-w-3]

Feel free to change it, I'm not sure I understand where the current version starts but I think it would make sense to do it in this PR

[oraNod]

On second thought I'm a little unsure about switching this to CalVer in this PR. It probably feels somewhat arbitrary to change it now so I'm inclined to go with this version until we have some actual helm chart improvements.

[oraNod]

I don't mind adding a dependency and calling yaml.safe_load(). It might be a little more robust.

[miles-w-3]

I wasn't sure whether we wanted to open the can of worms from the python standpoint, given all of the options around dependency management (venv, Dockerfile, etc)

[oraNod]

I'm happy to leave it to your preference. Maybe the fewer lines of code isn't worth the overhead of adding requirements files and all that.

[oraNod]

Here's my second round review. Thanks again @miles-w-3

[oraNod]

Maybe we can consider an alternative approach here, if you don't want to go with the yaml parser. I've been playing around with it a little. Here is what I've come up with:

# get the appVersion configured in Chart.yaml
def get_app_version():
    # naive implementation without yaml-parsing package
    try:
        with open("./.helm/starter/Chart.yaml") as chart:
            for line in chart:
                line = line.strip()
                if line.startswith('appVersion:'):
                    result = line.split(":", 1)
                    if len(result) != 2:
                        raise KeyError("Malformed appVersion")

                    version = result[1].rstrip()
                    if not version:
                        raise KeyError("No appVersion value found")

                    return version

        raise KeyError("Could not find appVersion")

    except FileNotFoundError:
        raise FileNotFoundError("Failed to open Chart.yaml")

Here I think using strip() might handle the whitespace better than replacing it. Plus we probably want to split on the first colon : only. Maybe it's a little pedantic though.

I also think it would be ideal to check for empty version values.

[miles-w-3]

@oraNod Thanks for the review, updated based on your feedbacl. If you confirm which version would be appropriate I can update that as well

[oraNod]

@oraNod Thanks for the review, updated based on your feedbacl. If you confirm which version would be appropriate I can update that as well

@miles-w-3 Thanks for updating! I think we're almost there. Made a couple of nit changes and left a comment about printing the pre-stripped result. We probably only need the one print call there and could make it a little more informative.

[miles-w-3]

@oraNod I didn't intend to leave either print in, removed both in my latest commit

[oraNod]

This looks good. Thanks for the great work here @miles-w-3 it's much appreciated. Glad to have someone from the community stepping forward as a maintainer on this project. I'll go ahead and merge.