fix(helm-requirements): remove the need for a Chart.yaml file

[eh-am]

Remove the need for the presence of a Chart.yaml file.

More context:
Mine is a bit of a special case. I have a custom format on top of helm, that just has a requirements.yaml file. I verified renovate works fine against it by commiting a dummy Chart.yaml. By looking at the code it seems that that file's contents don't make a difference, so I believe we could just remove its need from the codebase.

Closes #7547

[rarkins]

Hi, this needs discussion and agreement in an issue first.

[eh-am]

Makes sense, I assumed since it's a backwards compatible change we could skip the discussion process, but I was mistaken, my apologies. Here's the issue #7547

[rarkins]

Let's add some hardening around this part:

  deps = doc.dependencies.map((dep) => {
    const res: PackageDependency = {
      depName: dep.name,
      currentValue: dep.version,
    };

e.g. we should check that each dep contains both a name and version field.

[eh-am]

Hi @rarkins, I may have gone little overboard with the validations. Let me know what you think.

PS:
I haven't tried #7547 (comment) yet. That may invalidate the need for this PR.

[eh-am]

Hi @rarkins, any news on this? Unfortunately, the regex approach suggested by #7547 (comment) didn't work as well we would like (it requires a certain order which we guarantee).

[rarkins]

It's quite hard to evaluate this and be confident when there's so many changes. Is it possible to divide this into multiple PRs to merge one by one?

1. Add or change tests only with existing implementation passing.
2. Refactor existing implementation, leave tests unchanged.
3. Change implementation, add a few tests

The idea is to move as much of the changes as possible to refactoring PRs, where either (a) only tests change, or (b) only implementation changes. Then it's much easier to validate from inspection.

[rarkins]

Why was this test changed?

[eh-am]

Makes total sense. To be fair, my required changes are very simple: just remove a validation.

I think we can break into 2 steps:
1 - Remove the validation (looking up Chart.yaml) and the tests associated with that validation (latest commit)
2 - Harden the current implementation with #7544 (comment) and add associated tests, if necessary, more changes can be done, each on their own PR.

What do you think?

[rarkins]

Is there any reason not to add the hardening first?

[eh-am]

It's just that deleting code is easier than adding!

I started the change in a branch. I have few questions though:

1 - What to do when there's semantic versioning prefix (~^). Should we ignore them or skip altogether?

2 - There can be multiple reasons to skip a dependency (missing name, version, and repository). Is there a way to return all of them? If not, should we pick an order? Which one?
Example: name first, then version, then repository.

3 - Should we test for a specific skipReason, or just for its presence?
As a user, I find skipReason to be useful information, so maybe we should test for it?

4 - What's the stance on snapshot testing? I see them being used in multiple places, but it seems bit easy to overlook mistakes (I know I did).

[rarkins]

It's just that deleting code is easier than adding!

Yes, but my concern is that if we merge this first then we are "unhardened" and lead or warnings/errors unncecessarily.

I started the change in a branch. I have few questions though:

1 - What to do when there's semantic versioning prefix (~^). Should we ignore them or skip altogether?

If we don't support ranges like that yet then we need to add a skipReason

2 - There can be multiple reasons to skip a dependency (missing name, version, and repository). Is there a way to return all of them? If not, should we pick an order? Which one?
Example: name first, then version, then repository.

Yes, sounds fine

3 - Should we test for a specific skipReason, or just for its presence?
As a user, I find skipReason to be useful information, so maybe we should test for it?

Sure, skipReason should be deterministic in such cases (based on the order above)

4 - What's the stance on snapshot testing? I see them being used in multiple places, but it seems bit easy to overlook mistakes (I know I did).

It's certainly a trade-off to use snapshots. If you have the time to write them with literals it's much better because it's harder for someone in future to break them and everyone misses it (especially as diffs an be hard to read).

[eh-am]

Got It -> #8227

If we don't support ranges like that yet then we need to add a skipReason

Upon some quick investigation, it seems SkipReason.UnsuportedValue is already been set by the worker:

renovate/lib/workers/repository/process/lookup/index.ts

Line 376 in 255632f

res.skipReason = SkipReason.UnsupportedValue;

[eh-am]

Hi @rarkins,
Now that #8227 has been merged, are there any updates to be done there?

[rarkins]

@eh-am if you resolve conflicts and mark as ready for review then we can take a look again

[renovate-release]

🎉 This PR is included in version 24.42.0 🎉

The release is available on:

• GitHub release
• 24.42.0

Your semantic-release bot 📦🚀