astro-cli icon indicating copy to clipboard operation
astro-cli copied to clipboard

Use `semver.NewVersion` instead of `semver.MustParse`

Open neel-astro opened this issue 2 years ago • 0 comments

Describe the bug We are currently using the semver.MustParse method to parse versions in CLI. This would work fine if the version is static and defined in the code, but if the version string is passed in some form from the user end, then semver.MustParse will cause panic in case the version is invalid.

Ideally, we should avoid panic in our Go code, and so the idea is to use semver.NewVersion instead of semver.MustParse wherever we are getting the version string from the user's end, this would avoid code panics and give us errors which can be handled inside our code. Also if possible we should have a fallback in case of an error.

Ex: Current Code in docker.go Start method

if version := semver.MustParse(airflowVersion); version != nil {
    airflowDockerVersion = version.Major()
}

Expectation:

version, err := semver.NewVersion(airflowVersion)
if err != nil {
	airflowDockerVersion = 2 // fallback assuming it's Airflow 2, in order to handle the error
} else {
	airflowDockerVersion = version.Major()
}

What CLI Version did you experience this bug?

What Operating System is the above CLI installed on?

🪜 Steps To Reproduce

  1. In your local project's Dockerfile add an airflow version label with an invalid semver, ex: LABEL io.astronomer.docker.airflow.version="2.4.0.dev20220719"
  2. try astro dev start
  3. observer the panic in the code wrt semver library not being able to parse the image version

📸 Screenshots

neel-astro avatar Jul 21 '22 15:07 neel-astro