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

chore: upgrade kratos image to v1.2.0 #693

Merged
merged 3 commits into from
Jul 5, 2024
Merged

Conversation

meysam81
Copy link
Contributor

@meysam81 meysam81 commented Jul 4, 2024

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

Please correct the versions in chart.yaml and run make helm-docs :)

@@ -3,7 +3,7 @@ appVersion: "v1.1.0"
description: A ORY Kratos Helm chart for Kubernetes
name: kratos
icon: https://raw.githubusercontent.com/ory/docs/master/docs/static/img/logo-kratos.svg
version: 0.45.0
version: 0.46.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the version of the chart, that is autogenerated on release, please don't touch ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can update appVersion: "v1.1.0" to reflect the new version

Copy link
Contributor Author

@meysam81 meysam81 Jul 4, 2024

Choose a reason for hiding this comment

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

@Demonsthere

While I'm gonna go ahead and apply your suggestion, I think this is a good moment for me to ask, why would you upgrade all the charts at the same version, even if others have not had change?

Wouldn't you say it is more appropriate to have per-chart Chart-version? based on the changes of specific chart.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it can be confusing, it's a pattern that we created when all of the charts were in a active development model. We are unsure if we want to keep the charts in this repo, or move them to the corresponding application repos, so for now we just release them as a bunch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it helps in any way, I think the code structure of Helm charts is better on its own repository. That's usually more maintainable in terms of the frequency of code changes for application code as compared to infra/platform/IaC code.

Having one helm chart per repository is not feasible IMO, but I believe the current repository can be structured in a better way so that Chart versions are not bumped at the same time, each having their own pace.

It can result into confusion otherwise when one of the charts need a bump and others don't, and yet all get a bump anyway!

@Demonsthere Demonsthere merged commit cafddf1 into ory:master Jul 5, 2024
11 checks passed
@meysam81 meysam81 deleted the patch-1 branch July 5, 2024 11:38
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.

2 participants