Skip to content

Fix env variable and allow additional config#79

Open
mladedav wants to merge 4 commits into
kellnr:mainfrom
mladedav:main
Open

Fix env variable and allow additional config#79
mladedav wants to merge 4 commits into
kellnr:mainfrom
mladedav:main

Conversation

@mladedav

Copy link
Copy Markdown

Two small changes, but I can split the PR if you prefer.

  • env variables should be alphanumeric with underscores. We use crate registry name with a hyphen which creates a malformed env variable for the token and doc building dosn't work. Simple workaround is to use underscores in our name but that's not consistent how we use it elsewhere so this is a failsafe.
  • we migrated from another registry and pushed all crates from there into kellnr. Now I would like kellnr to build the docs but while we've fixed the dependencies in json manifests, the raw content still points the dependencies to the old registry. We would like to add to the docs builder config.toml lines that would replace that registry with the local one (similarly to what kellner does to itself) but there is no way to extend that config toml right now.

The second change might be contentious and I am open to other possibilities. We could just add a token for now but we would also like to eventually sunset the old registry and not keep it around indefinitely for this. We could replace the contents of all the packages but if we don't have to, I'd rather not.

@secana

secana commented Jun 12, 2026

Copy link
Copy Markdown
Member

Thanks, two nice changes.

One thing on the config append: a plain substitution only indents the first line, so a multi-line configMapExtraConfig (like your [source.crates-io] + replace-with) would break the YAML block scalar. Could you use
nindent so every line gets indented?

  {{- .Values.docBuilder.configMapExtraConfig | nindent 4 }}

The escape-hatch approach itself is fine by me, consistent with extraEnvVars elsewhere. Also needs a Chart.yaml version bump or CI lint will fail, and a quick README line for the new value.

@mladedav

Copy link
Copy Markdown
Author

Right, fixed the indentation issue, thanks for catching that. I didn't copy the leading - from your suggestion though but I've verified that it works either way. This way there are two empty lines (not sure where the second is coming from) between the last hardcoded line and the user-supplied block. With the leading hyphen, there are no empty lines between the two so this feels slightly more readable. But if there's something I'm overlooking or you'd just prefer the othe way, I'll happily change it to discard the whitespace.

Do you have any specific place in mind where to mention this in the readme? I noticed it also work a bit as a changelog though it seems to mention just breaking changes users should be aware of when upgrading so I assume that's not the place? Should I add another section at the end for docbuilder?

In either case, I'll wait with this PR on the other one, that one seemed to be closer and the version bump would just create a conflict. As well as the readme change possibly if I am to put it at the end as a new section.

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.

2 participants