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

Helm dependencies should optionally bump chart.yaml #4728

Open
adusumillipraveen opened this issue Oct 25, 2019 · 37 comments · Fixed by #26441
Open

Helm dependencies should optionally bump chart.yaml #4728

adusumillipraveen opened this issue Oct 25, 2019 · 37 comments · Fixed by #26441
Assignees
Labels
auto:reproduction A minimal reproduction is necessary to proceed help wanted Help is needed or welcomed on this issue manager:helm Helm package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)

Comments

@adusumillipraveen
Copy link
Contributor

What would you like Renovate to be able to do?
It is great that renovate supports helm dependency management now. But, it would be great if renovate can optionally bump the chart version in chart.yaml in the PR as the new chart needs to be published as a new vesion.

Describe the solution you'd like
Renovate should (optionally ) bump the chart version in Chart.yaml when raising a PR to bump requirements.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Currently one way is to manually edit it so that we can publish a new version of chart.

Additional context
NA

@JamieMagee JamieMagee added manager:helm Helm package manager needs-requirements priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others labels Oct 27, 2019
@JamieMagee
Copy link
Contributor

Yeah, this should be possible. We already have this functionality for bumping the version in package.json using the bumpVersion option, so extending this to work with helm shouldn't be too difficult.

@JamieMagee JamieMagee added help wanted Help is needed or welcomed on this issue ready and removed needs-requirements labels Oct 27, 2019
@rarkins rarkins added the type:feature Feature (new functionality) label Mar 6, 2020
@rarkins rarkins removed the ready label Jun 18, 2020
@rarkins rarkins added the auto:reproduction A minimal reproduction is necessary to proceed label Aug 4, 2020
@travisgroth
Copy link

Hi @rarkins I'd love to see this feature included. Can I help with the requirements or testing at all?

Functionality like https://docs.renovatebot.com/configuration-options/#bumpversion would be great.

To implement this, the version field in Chart.yaml needs the increment.

@rarkins
Copy link
Collaborator

rarkins commented Sep 29, 2020

@travisgroth which of our managers would this be applicable to? e.g. helmv3?

It will be a little bit tricky because npm's updating approach is quite unique compared to other managers and so therefore you can't simply copy over the way it does bumpVersion too.

I think we'd need to add an optional new function to managers with same or similar signature as this one in npm:

export function bumpPackageVersion(
content: string,
currentValue: string,
bumpVersion: ReleaseType | string
): string {
if (!bumpVersion) {
return content;
}
logger.debug(
{ bumpVersion, currentValue },
'Checking if we should bump package.json version'
);
let newPjVersion: string;
try {
if (bumpVersion.startsWith('mirror:')) {
const mirrorPackage = bumpVersion.replace('mirror:', '');
const parsedContent = JSON.parse(content);
newPjVersion =
(parsedContent.dependencies || {})[mirrorPackage] ||
(parsedContent.devDependencies || {})[mirrorPackage] ||
(parsedContent.optionalDependencies || {})[mirrorPackage] ||
(parsedContent.peerDependencies || {})[mirrorPackage];
if (!newPjVersion) {
logger.warn('bumpVersion mirror package not found: ' + mirrorPackage);
return content;
}
} else {
newPjVersion = inc(currentValue, bumpVersion as ReleaseType);
}
logger.debug({ newPjVersion });
const bumpedContent = content.replace(
/("version":\s*")[^"]*/,
`$1${newPjVersion}`
);
if (bumpedContent === content) {
logger.debug('Version was already bumped');
} else {
logger.debug('Bumped package.json version');
}
return bumpedContent;
} catch (err) {
logger.warn(
{
content,
currentValue,
bumpVersion,
},
'Failed to bumpVersion'
);
return content;
}
}

Then we'd need to work out where/how to call it, potentially here:

if (!newContent) {

@travisgroth
Copy link

@travisgroth which of our managers would this be applicable to? e.g. helmv3?

My immediate use case is for helm-values, but I believe you'd want the option on any changes to a chart. So:

  • helmv3
  • helm-requirements
  • helm-values

@mikebryant
Copy link
Contributor

I've got an implementation of this going for helmv3 at #7670 :)

@rarkins rarkins added the status:requirements Full requirements are not yet known, so implementation should not be started label Jan 12, 2021
@HonkingGoose HonkingGoose added auto:reproduction A minimal reproduction is necessary to proceed and removed auto:reproduction A minimal reproduction is necessary to proceed labels Mar 3, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2021

Hi there,

The Renovate team needs your help! To fix the problem, we first need to know exactly what's causing the bug. A minimal reproduction help us to pinpoint the exact cause of the bug.

To get started, please read our guide on minimal reproductions to understand what is needed.

We may close the issue if you have not provided a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@multani
Copy link
Contributor

multani commented Jan 4, 2022

I think this has been implemented in #7670 already, no?

@travisgroth
Copy link

@multani not completely. The chart version isn't bumped by the helm-values manager AFACT. The practical result is that if you bump your image version, the chart metadata isn't updated. Only updates to your chart's dependencies properly bump the chart version.

You can see the difference in these two PRs:

https://github.com/pomerium/pomerium-helm/pull/232/files (helmv3 manager update)
https://github.com/pomerium/pomerium-helm/pull/239/files (I believe this is a helm-values update)

@travisgroth
Copy link

@rarkins @HonkingGoose any chance of this getting some attention?

@rarkins
Copy link
Collaborator

rarkins commented Jan 29, 2022

@travisgroth the issue labeled with help wanted and reproduction:needed, so unfortunately not. Once there's a minimal reproduction and we think the requirements are clear then we'll update to status:ready, after which a community contribution would be needed

@roobre
Copy link

roobre commented May 6, 2022

Hi @rarkins,

I am interested in seeing this feature happen for the helm-values manager, and me or maybe one of my teammates should be able to contribute with a PR, hopefully covering the "community contribution would be needed" part of your comment.

Is there any way we can help with the "Once there's a minimal reproduction" part? Should a PR made by helm right now be sufficient, or were you thinking about something else?

Thanks!

@rarkins
Copy link
Collaborator

rarkins commented May 6, 2022

Hi @roobre, a PR would be most welcome. You're also welcome to jump straight to the PR if you're confident in it. Normally we recommend for contributor's sakes to create a reproduction and discuss how it would work within the issue first, just to reduce the chance of misunderstandings.

@roobre
Copy link

roobre commented May 6, 2022

Hey @rarkins,
I totally get it, we do the same for our OSS projects 😄

In this case we might jump straight to the PR as you suggest because I think the base functionality is pretty much what the title of this issue says. Of course we can discuss any detail in the PR :)

I'll add this to the team's backlog and see if we can get to work on it soon!

@rarkins rarkins added reproduction:provided and removed auto:reproduction A minimal reproduction is necessary to proceed status:requirements Full requirements are not yet known, so implementation should not be started labels May 15, 2023
@viceice
Copy link
Member

viceice commented May 17, 2023

is helm values manager for the EOL helm version V2?

@roobre
Copy link

roobre commented May 17, 2023

@viceice As far as I can tell helm-values is still useful for helm 3. helm-requirements is the one that is probably not useful with Helm version 3 and onwards, as it targets a file called requirements.yaml which is no longer recommended with the new format for Chart.yaml that Helm versiom 3 introduced.

@secustor
Copy link
Collaborator

No, helm-values is used for both. V2 & V3

helm-requirements is only valid for V2

@jessebot
Copy link

jessebot commented Sep 6, 2023

is there additional testing required to move this forward after @roobre's comment, #4728 (comment) ?

@rarkins
Copy link
Collaborator

rarkins commented Sep 6, 2023

This issue has label status:ready which means PRs are welcome

@kvanzuijlen
Copy link
Contributor

@rarkins I'd be willing to take a crack at this, as it would also help out big time for what I'm working on with jenkinsci/helm-charts#975. I hope/think I can come up with something before the end of the year. I'll keep you posted!

@renovate-release

This comment has been minimized.

@rarkins rarkins reopened this Jan 20, 2024
@kastl-ars
Copy link

kastl-ars commented May 31, 2024

OK, what is the state on this one? It was closed and reopened in January?

Apparently reverted in #26758

@aairey

This comment has been minimized.

@secustor

This comment was marked as resolved.

@rarkins rarkins added the auto:reproduction A minimal reproduction is necessary to proceed label Aug 27, 2024
Copy link
Contributor

Hi there,

Get your discussion fixed faster by creating a minimal reproduction. This means a repository dedicated to reproducing this issue with the minimal dependencies and config possible.

Before we start working on your issue we need to know exactly what's causing the current behavior. A minimal reproduction helps us with this. Discussions without reproductions are less likely to be converted to Issues.

Please follow these steps:

  1. Read our guide on creating a minimal reproduction.
  2. Go to our minimal reproduction template repository.
  3. Select the Use this template button to create a new repository based on the template.
  4. Work on the minimal reproduction in your own repository.
  5. Fill out the information in your repository's README.md.
  6. Add the link to your reproduction to the first post of your Discussion. If you are not the original author, you can post a new comment with the link.

Good luck,

The Renovate team

@rarkins
Copy link
Collaborator

rarkins commented Aug 27, 2024

Hi, could someone who's interested in this feature create a reproduction which follows our above guidelines? e.g. including a readme explaining observed vs expected behavior.

I think the best way to do this would be to:

  • Create the minimal reproduction repo
  • Install the renovate bot into it, to create a PR
  • Add a commit on top showing the "bump" version which would be expected (this should also cause Renovate to stop updating the PR, keeping it reproduced for eternity)

@roobre

This comment was marked as off-topic.

@aairey

This comment was marked as off-topic.

@secustor

This comment was marked as off-topic.

@LeoColomb
Copy link

@rarkins I've just made a minimal reproduction repo: https://github.com/LeoColomb/renovate-helm-values
As requested, some README to guide and an additional commit to highlight the missing bump on the PR (LeoColomb/renovate-helm-values#1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:reproduction A minimal reproduction is necessary to proceed help wanted Help is needed or welcomed on this issue manager:helm Helm package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.