Skip to content

Reimplement component - #16

Open
vshntea wants to merge 1 commit into
projectsyn:mainfrom
vshntea:reimplement
Open

vshntea wants to merge 1 commit into
projectsyn:mainfrom
vshntea:reimplement

Conversation

@vshntea

@vshntea vshntea commented Sep 25, 2026

Copy link
Copy Markdown

This repository hasn't been touched in 5 years. Changes have been made from the ground up, I only found out about this component after it was implemented locally.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update the documentation.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Link this PR to related issues.

@vshntea
vshntea force-pushed the reimplement branch 2 times, most recently from c2bc958 to 4c50fc5 Compare September 25, 2026 13:54
Comment thread .github/workflows/release.yaml Resolved
Comment thread .github/workflows/release.yaml Resolved
Comment thread .github/workflows/test.yaml Resolved
This repository hasn't been touched in 5 years. Changes have been made
from the ground up, I only found out about this component after it was
implemented locally.
Comment thread class/defaults.yml
Comment on lines +11 to +12
charts:
chart1:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's no need to blindly adhere to Project Syn best practices when you sidestep commodore-renovate's Helm chart support anyway by using a regex manager.

Use an alias for the component.
The parent key for all of the following parameters is `helm_chart`.

The component is multi-tenant, multi-instance and multi-version aware. Each

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please keep docs at one sentence per line.

Comment on lines +5 to +7
The component is multi-tenant, multi-instance and multi-version aware. Each
instance is compiled with `--alias <instance>` and can override the parameters
below.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Second sentence is mostly irrelevant to anyone using the component.

Comment thread component/app.jsonnet
Comment on lines 13 to 15
{
[instance]: app,
['%s/%s' % [ appPath, instance ]]: app,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd probably make not instantiating this component an error, something along the lines of doing

if instance == 'helm-chart' then
  error 'Component helm-chart must be instantiated with `helm-chart as foo` when using it.'
else {
  ['%s/%s' % [ appPath, instance ]: app,
}

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.

3 participants