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(helm): move earliestReleaseDate to _common-metadata.tpl #3428

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

corneliusroemer
Copy link
Contributor

Followup to #3380

Hard coded metadata fields are configured in _common-metadata.tpl as they are not something to be configured per instance via values.yaml

Disabling per organism isn't easy for common-metadata fields, so let's remove the ability to disable, earliest release is something that can always be calculated so no need to disable it. Let's only make configurable what truly needs to be configurable.

resolves #

preview URL:

Summary

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

Followup to #3380

Hard coded metadata fields are configured in _common-metadata.tpl as they are not something to be configured per instance via values.yaml

Disabling per organism isn't easy for common-metadata fields, so let's remove the ability to disable, earliest release is something that can always be calculated so no need to disable it. Let's only make configurable what truly needs to be configurable.
Copy link
Member

@chaoran-chen chaoran-chen left a comment

Choose a reason for hiding this comment

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

Could you please explain more why this should be not disableable? I don't see how the current code is problematic and I think that the feature is useful. I can well imagine using it very soon for one of the wastewater instances.

The classification as chore(helm) also seems quite wrong as it is a breaking change that removes a feature.

@corneliusroemer corneliusroemer added the preview Triggers a deployment to argocd label Dec 11, 2024
@corneliusroemer
Copy link
Contributor Author

Well, I never approved the original PR.

earliestReleaseDate is a magic field name, hence it should live in the _common-metadata.tpl like other computed fields.

The argument must be the other way round, why should it be disableable, this wasn't part of the original discussion so it's an extra feature that was added. What harm is done?

Disabling at per-organism level is not straightforward with current setup of common-metadata. What would work is disable entirely for the entire instance, which should be enough.

@theosanderson
Copy link
Member

theosanderson commented Dec 11, 2024

My 2 cents:

I agree with Cornelius that the current version doesn't fit into our logic so well.

Nevertheless, it doesn't seem a big problem for me, and if it's going to be useful for Chaoran the current behaviour seems ok for me for now.

I think we should aim to make "common metadata" items configurable in terms of the editable parameters like whether they appear in tables are searchable and stuff relatively soon, and would support a change somewhat like this after that.

@chaoran-chen
Copy link
Member

this wasn't part of the original discussion so it's an extra feature that was added

@corneliusroemer, that's not true, we discussed it a lot in #2894. You didn't respond at the end to the question whether adding the disabling is acceptable for you but didn't object either but said that the issue was ready-to-go.

Disabling at per-organism level is not straightforward with current setup of common-metadata. What would work is disable entirely for the entire instance, which should be enough.

An instance-wide disabling would be fine with me as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants