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

feat: add labels for helm release #1046

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

Conversation

yxxhero
Copy link
Member

@yxxhero yxxhero commented Sep 29, 2023

@yxxhero yxxhero changed the title feat: add labels for k8s resources feat: add labels for helm release Sep 29, 2023
@yxxhero yxxhero marked this pull request as ready for review September 29, 2023 12:48
@@ -2563,6 +2571,8 @@ func (st *HelmState) flagsForUpgrade(helm helmexec.Interface, release *ReleaseSp

flags = st.appendHelmXFlags(flags, release)

flags = st.appendLabelsFlags(flags, helm, release)
Copy link
Contributor

Choose a reason for hiding this comment

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

The release label in helmfile only allows users to choose release. Is it necessary to add it to helm installation parameters?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stale
Copy link

stale bot commented Oct 28, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Oct 28, 2023
@yxxhero yxxhero removed the wontfix This will not be worked on label Oct 28, 2023
@yxxhero yxxhero linked an issue Nov 12, 2023 that may be closed by this pull request
@yxxhero
Copy link
Member Author

yxxhero commented Nov 14, 2023

@mumoshu WDYT? Looking forward to your feedback. thanks so much.

Copy link

stale bot commented Nov 28, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 28, 2023
@stale stale bot closed this Dec 5, 2023
@yxxhero yxxhero reopened this Dec 5, 2023
@stale stale bot removed the wontfix This will not be worked on label Dec 5, 2023
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
Signed-off-by: yxxhero <[email protected]>
@yxxhero
Copy link
Member Author

yxxhero commented Dec 6, 2023

@mumoshu

{
name: "empty label value",
labels: map[string]string{"foo": ""},
want: "foo=null",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
want: "foo=null",
want: "foo=",

Probably we want this presuming "null" and ""(empty string) are two different things and it's actually an empty string here

for _, k := range keys {
val := labels[k]
if val == "" {
val = "null"
Copy link
Contributor

Choose a reason for hiding this comment

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

func (st *HelmState) SelectReleases(includeTransitiveNeeds bool) ([]Release, error) {
values := st.Values()
rs, err := markExcludedReleases(st.Releases, st.Selectors, st.CommonLabels, values, includeTransitiveNeeds)
rs, err := markExcludedReleases(st.Releases, st.Selectors, values, includeTransitiveNeeds)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm.. not ideal but probably we'd better do assign releases with labels here so that we won't accidentally miss calling GetReleasesWithLabels()

Suggested change
rs, err := markExcludedReleases(st.Releases, st.Selectors, values, includeTransitiveNeeds)
st.Releases = st.GetReleasesWithLabels()
rs, err := markExcludedReleases(st.Releases, st.Selectors, values, includeTransitiveNeeds)

Copy link
Contributor

Choose a reason for hiding this comment

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

And make it a private func getReleasesWithLabels so that it is clearer that the function is not supposed to be called by the consumer of State

@tunahansezen
Copy link
Contributor

tunahansezen commented Dec 29, 2023

Any update on this? @yxxhero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

commonLabels not applied on releases
4 participants