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.

Sign up for GitHub

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

Jump to bottom

controllers: Fix helmchart values file merge test #492

Closed
wants to merge 23 commits into from
Closed

controllers: Fix helmchart values file merge test #492

wants to merge 23 commits into from

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Nov 18, 2021 •
edited
Loading

Test case "Setting valid valuesFile attribute" and the tests around it
aren't isolated and most of the time pass because of the results from
the previous tests being re-read as the test expectation match the
previous test results. Failures are very rare to reproduce, even in
the CI they aren't seen but it failed very frequently on my computer,
especially this specific case because unlike the other cases, there is
just one file to be merged, which invalidates the chart result from
the previous cases.
In order to ensure that the test waits for the chart to be updated by its
action and not by any other previous updates, status condition message
seems to be the most reliable way, as it also contains the paths of the
files that were merged.
With this change, I could no longer reproduce the failure on my
computer.
Reordering the tests makes this issue more clear.

• Failure [17.115 seconds]
HelmChartReconciler
~/go/src/github.com/fluxcd/source-controller/controllers/helmchart_controller_test.go:52
  HelmChart from GitRepository
  ~/go/src/github.com/fluxcd/source-controller/controllers/helmchart_controller_test.go:554
    Creates artifacts for [It]
    ~/go/src/github.com/fluxcd/source-controller/controllers/helmchart_controller_test.go:582

    Expected
        <bool>: true
    to be false

    ~/go/src/github.com/fluxcd/source-controller/controllers/helmchart_controller_test.go:840

Since the reconcilers-dev branch has all the new tests with new
structures, I'm not sure if we need to make these tests any better.
They do validate the results, but fail sometimes on some machines.

NOTE: This depends on the changes in #485.

hiddeco and others added 23 commits November 18, 2021 15:50
@hiddeco
internal/helm: add helpers to load chart metadata
3f116cf 
This commits adds `LoadChartMetadataFromArchive` and
`LoadChartMetadataFromDir` helpers to the internal `helm` package
to be able to make observations to the Helm metadata file without
loading the chart in full.

The helpers are compatible with charts of the v1 format (with a
separate `requirements.yaml` file), and an additional
`LoadChartMetadata` helper is available to automatically call the
right `LoadChartMetadataFrom*` version by looking at the file
description of the given path.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: add repository cache helpers
373a035 
This commits adds simple caching capabilities to the
`ChartRepository`, which makes it possible to load the `Index` from a
defined `CachePath` using `LoadFromCache()`, and to download the index
to a new `CachePath` using `CacheIndex()`.

In addition, the repository tests have been updated to make use of
Gomega, and some missing ones have been added.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: optimize dependency manager
b496241 
This commit starts with the optimization of the `DepenendencyManager`,
ensuring the chart indexes are lazy loaded, and replacing the
(limitless) concurrency with a configurable number of workers with a
default of 1.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: introduce ChartBuilder
14db843 
This commit starts with the creation of a `ChartBuilder` to facilitate
the (conditional) build of a chart outside of the reconciler logic.

The builder can be configured with a set of (modifying) options, which
define together with the type of chart source what steps are taken
during the build.

To better facilitate the builder's needs and attempt to be more
efficient, changes have been made to the `DependencyBuilder` and
`ChartRepository` around (order of) operations and/or lazy-load
capabilities.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: wire ChartRepository in reconciler
19714e4 
This wires the `ChartRepository` changes into the reconciler to ensure
it works.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: make ChartBuilder an interface
79edb73 
This commit refactors the `ChartBuilder` that used to be a do-it-all
struct into an interace with two implementations:

- `LocalChartBuilder`: to build charts from a source on the local
  filesystem, either from a directory or from a packaged chart.
- `RemoteChartBuilder`: to build charts from a remote Helm repository
  index.

The new logic within the builders validates the size of the Helm size
it works with based on the `Max*Size` global variables in the internal
`helm` package, to address the recommendation from the security audit.

In addition, changes `ClientOptionsFromSecret` takes now a directory
argument which temporary files are placed in, making it easier to
perform a garbage collection of the whole directory at the end of a
reconcile run.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: rough wiring of Helm chart builder
6c8abe3 
This commit starts wiring the factored out Helm chart build logic into
the reconciler to ensure, validating the API capabilities.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: divide into subpackages
1f2b37f 
With all the logic that used to reside in the `controllers` package
factored into this package, it became cluttered. This commit tries to
bring a bit more structure in place.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: more tidying of wiring
2ac1277 
Dealing with some loose ends around making observations, and code
style.

The loaded byes of a chart are used as a revision to ensure e.g.
periodic builds with unstable ordering of items do not trigger a false
positive.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
internal/helm: local builder & dep manager test
0236a32 
Add more chart local builder and dependency manager tests.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
internal/helm: add remote builder tests
934c214 
- For remote builds, if the build option has a version metadata, the
  chart should be repackaged with the provided version.
- Update internal/helm/testdata/charts/helmchart-0.1.0.tgz to include
  value files for testing merge chart values.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
internal/helm: add cached chart build tests
d67065d 
Cached chart build tests for both local and remote builder.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco
internal/helm: tweak and test chart build summary
0c7744a 
This makes the string less verbose and deals with the safe handling
of some edge-case build states.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: add more tests
6143fdb 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: introduce typed BuildError
7965ccc 
This commit introduces a typed `BuildError` to be returned by
`Builder.Build` in case of a failure.

The `Reason` field in combination with `BuildErrorReason` can be used
to signal (or determine) the reason of a returned error within the
context of the build process.

At present this is used to determine the correct Condition Reason, but
in a future iteration this can be used to determine the negative
polarity condition that should be set to indicate a precise failure to
the user.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
internal/helm: test load funcs for max size cases
ee7184b 
This includes a change of the defaults to more acceptible (higher)
values.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco
internal/helm: "value files" -> "values files"
fb12091 
Previous usage while consistent, was incorrect, and inconsitent with
the field in the API spec.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Allow configuration of Helm file limits
e68e8bc 
This allows custom configuration of the Helm file read limits, allowing
a user to overwrite them to their likenings if the defaults are too
restrictive for their specific setup using arguments:

`--helm-{index,chart,chart-file}-limit`

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: various nitpicks
5b028a1 
- Add some more documentation around chart builders
- Ensure correct indentation in some doc comments
- Provide example of using `errors.Is` for typed `BuildError`
- Mention "bytes" in file size limit errors
- Add missing copyright header

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
helm/internal: add ErrChartReference
4ebd742 
This makes it possible to signal reference (validation) errors
happening before the build process actually starts dealing with
the chart.

At present, this does not have a more specific counterpart in the API,
but this is expected to change when the conditions logic is revised.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: set generation as version metadata
8a7f486 
By providing the Generation of the object that is getting reconciled
as version metadata to the builder if any custom values files are
defined, the Artifact revision changes if the specification does,
ensuring consumers of the Artifact are able to react to changes in
values (and perform a release).

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
internal/helm: ensure cached chart name matches
ae86aab 
This helps detect e.g. path or chart name reference changes.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz
controllers: Fix helmchart values file merge test
dda029b 
Test case "Setting valid valuesFile attribute" and the tests around it
aren't isolated and most of the time pass because of the results from
the previous tests being re-read as the test expectation match the
previous test results. Failures are very rare to reproduce, even in
the CI they aren't seen but it failed very frequently on my computer,
especially this specific case because unlike the other cases, there is
just one file to be merged, which invalidates the chart result from
the previous cases.
In order to ensure the test wait for the chart to be updated by its
action and not by any other previous updates, status condition message
seems to be the most reliable way, as it also contains the paths of the
files that were merged.
With this change, I could no longer reproduce the failure on my
computer.
Reordering the tests makes this issue more clear.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco hiddeco force-pushed the helmchart-reconciler-dev branch 4 times, most recently from 3594188 to 2392326 Compare November 19, 2021 16:16
@hiddeco hiddeco deleted the branch fluxcd:helmchart-reconciler-dev November 22, 2021 09:30
@hiddeco hiddeco closed this Nov 22, 2021
@darkowlzz darkowlzz mentioned this pull request Nov 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@darkowlzz @hiddeco