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: generate simple generic helm chart #665

Merged
merged 42 commits into from
Aug 17, 2023
Merged

feat: generate simple generic helm chart #665

merged 42 commits into from
Aug 17, 2023

Conversation

csviri
Copy link
Contributor

@csviri csviri commented Jul 20, 2023

This is the MVP for helm chart generator, it is "bare minimum" that works, will add subsequent PRs to improve.

@csviri csviri changed the title Helm chart feat: generate simple generic helm chart Jul 20, 2023
@csviri csviri self-assigned this Jul 24, 2023
@csviri csviri linked an issue Jul 27, 2023 that may be closed by this pull request
@csviri csviri marked this pull request as ready for review August 16, 2023 15:24
import io.dekorate.kubernetes.config.BaseConfigFluent;
import io.dekorate.kubernetes.config.Configurator;

public class DisableDefaultHelmListener extends Configurator<BaseConfigFluent<?>> implements WithSession {
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment. it is to disable default helm generator

/**
* Can be used to disable helm chart generation.
*/
@ConfigItem(defaultValue = "true")
Copy link
Member

Choose a reason for hiding this comment

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

This should be false by default, imo. We don't want to generate Helm charts on every build for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK, I would rather generate everything, users still can opt out, if something is not needed, but they would be aware that it's there. Also would worth to measure the time difference. Will do some naive measurement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Briefly did some "naive" measurement on exposed app sample, the problem is that the build time varies even if nothing changes around ~300ms (without tests), independently from the fact that helm genertion is turned on or off. But basically comparing it with or without helm there was no significant difference.

Let's assume that there is a difference in the build, thus there is additional 100ms more if helm is enabled (while the project builds without tests in about 11s), is this worth turning it off? it's really not significant imo

Copy link
Member

Choose a reason for hiding this comment

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

This is more important in the dev mode, so at the very least, helm chart generation should be disabled in dev mode, though I still think it shouldn't be active by default, the same way we don't generate bundles by default either (which is why the support is in a different module). Since helm support is in the core module, it should be deactivated by default. Generating something all the time just to make people aware of it is a bad idea, imo. This should be addressed by documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK, can do it for now, but my opinion is that if there is no explicit reason why not to generate it it should be there, even for dev mode, so users can see the helm chart if they add an additional dependent resource.

Why bundle generation is a separate module, I mean why it is not just a flag to not generate it?

Copy link
Member

Choose a reason for hiding this comment

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

IDK, can do it for now, but my opinion is that if there is no explicit reason why not to generate it it should be there, even for dev mode, so users can see the helm chart if they add an additional dependent resource.

The core principle is to be as efficient as possible so sure, we can always say things are "fast" enough but from my point of view, if something doesn't need to be done all the time, it shouldn't.

Why bundle generation is a separate module, I mean why it is not just a flag to not generate it?

Cleaner separation of concerns. Also allows to not bring the dependencies if not used. For that matter, we might want to extract Helm support in its own module at some later point as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaner separation of concerns. Also allows to not bring the dependencies if not used. For that matter, we might want to extract Helm support in its own module at some later point as well.

I think this is a matter of definition, if having such a features in different package and feature flag vs different module. I personally definitely would prefer just feature flag, especially since there should be this nice unified way how we configure quarkus.


@BuildStep
public void handleHelmCharts(
BuildProducer<ArtifactResultBuildItem> dummy,
Copy link
Member

Choose a reason for hiding this comment

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

If this is needed to ensure this steps runs after another one, please add a comment to that effect, otherwise that parameter might get removed at some point, leading to possibly difficult to diagnose issues.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is a BuildProducer that isn't used so I don't think there's any point in keeping this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without it it does not gets executed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe there is a more elegant way to do this

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

I see that you have followed the approach of using templates instead of YAML expressions which is perfectly fine and I think it fits better in your use case.

I've added a few comments about the tests, but not blockers at all.

Good job!!

(jar) -> jar.addClasses(SimpleReconciler.class, SimpleCR.class, SimpleSpec.class, SimpleStatus.class));

@Test
void generatesHelmChart() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to assert that the resources are indeed generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will take a look on this, there was an issue with the test, and though an e2e will be enough, bit will give it an other try. Thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added simple asserts, not sure if we should check the templates in depth in this case.

// https://github.com/dajudge/kindcontainer/issues/235
@Disabled
@QuarkusTest
class HelmDeploymentE2ETest {
Copy link
Contributor

Choose a reason for hiding this comment

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

A while ago, I remember having a workflow that used Kind to install the ping pong operators. It had been useful to use this workflow to verify the helm installation via CLI instead of using junit test which is prone to errors (because it needs maintainers to have installed certain tools like helm, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is quite annoying when you need those tools, but managing such tests is quite handy from java code. Will take a look on this in next pr. An other possibility is to commit something like this script to the repo:
https://helm.sh/docs/intro/install/#from-script
And install helm during the test if not installed yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For GitHub Actions there is a nice action that does that.

@csviri csviri requested a review from metacosm August 17, 2023 10:53
try (InputStream file = Thread.currentThread().getContextClassLoader()
.getResourceAsStream(CRD_ROLE_BINDING_TEMPLATE_PATH)) {
if (file == null) {
throw new IllegalArgumentException("Template file " + CRD_ROLE_BINDING_TEMPLATE_PATH + " doesn't exist");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not much benefit to check for null on a static resource.

@metacosm metacosm merged commit 1515cb7 into main Aug 17, 2023
@metacosm metacosm deleted the helm-chart branch August 17, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate Helm Chart for an Operator (MVP)
3 participants