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

Add config option to set maximum Istio version #507

Closed
wants to merge 2 commits into from

Conversation

dgn
Copy link
Collaborator

@dgn dgn commented Dec 4, 2024

No description provided.

@dgn dgn requested a review from a team as a code owner December 4, 2024 13:09
@dgn dgn force-pushed the configure-maximum-istio-version branch from 1e29861 to e74df6c Compare December 4, 2024 13:18
@dgn
Copy link
Collaborator Author

dgn commented Dec 5, 2024

It looks like the pre-release versions are not accepted by the semver regex. Which is strange, I think they should be fine

nevermind this is about latest

Copy link
Contributor

@luksa luksa left a comment

Choose a reason for hiding this comment

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

Hmm, this approach won't work, because spec.version is currently not really an actual version number. It's just a string (for example, when spec.version: latest, latest is just the name, whereas the actual version number is whatever is set in the version field in versions.yaml). But the operator currently doesn't even read the versions.yaml file.

The operator currently doesn't really know anything about versions. It simply takes the spec.version string and uses it to construct the path for the chart it must render.

We either need to remove the version names from versions.yaml and just use the version number as the version name (but this prevents us from using "latest" as the version name), or we make the operator read the versions.yaml file, allowing it to map the version name to the version number.

return reconciler.NewValidationError("spec.version is not a valid semver: " + err.Error())
}
if config.Config.MaximumIstioVersion != nil && istioVersion.GreaterThan(config.Config.MaximumIstioVersion) {
return reconciler.NewValidationError("spec.version is not supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the version to the error message. This makes it read as though setting the field itself is not supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

@luksa
Copy link
Contributor

luksa commented Dec 5, 2024

Actually, we could get rid of the version names by simply defaulting an empty spec.version to whatever the latest version is. Although, without versions.yaml, the operator would have to read the directory names and find the highest semver, which is obviously not the best approach. So, we definitely need to pass the versions.yaml file to the operator, so we don't really need to get rid of the version names.

@dgn dgn force-pushed the configure-maximum-istio-version branch from e74df6c to b8b1cb6 Compare December 5, 2024 13:49
@dgn
Copy link
Collaborator Author

dgn commented Dec 5, 2024

Hmm, this approach won't work, because spec.version is currently not really an actual version number. It's just a string (for example, when spec.version: latest, latest is just the name, whereas the actual version number is whatever is set in the version field in versions.yaml). But the operator currently doesn't even read the versions.yaml file.

The operator currently doesn't really know anything about versions. It simply takes the spec.version string and uses it to construct the path for the chart it must render.

We either need to remove the version names from versions.yaml and just use the version number as the version name (but this prevents us from using "latest" as the version name), or we make the operator read the versions.yaml file, allowing it to map the version name to the version number.

I think this is good and we should do it as part of implementing version aliases. latest then just becomes an alias for the latest version in versions.yaml.

For this specific instance, we just need to be able have an upper limit on the version, so IMO it would be a bit overkill to implement this (and then revise it anyway when implementing version aliases).

@dgn dgn force-pushed the configure-maximum-istio-version branch from b8b1cb6 to 8cb2baf Compare December 5, 2024 13:55
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.50%. Comparing base (5af33d3) to head (10ab74e).
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   78.21%   78.50%   +0.29%     
==========================================
  Files          38       38              
  Lines        2272     2294      +22     
==========================================
+ Hits         1777     1801      +24     
+ Misses        404      403       -1     
+ Partials       91       90       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dgn
Copy link
Collaborator Author

dgn commented Dec 13, 2024

Closing this as per our discussion. We'll use a normal branching strategy and re-evaluate after a few minor releases

@dgn dgn closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants