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

Feature request: Ability to specify ConfigMap instead of startupScriptContent for init scripts #85

Open
sobotklp opened this issue May 4, 2023 · 7 comments
Labels
type: enhancement Improvement to usability or performance

Comments

@sobotklp
Copy link

sobotklp commented May 4, 2023

There are some drawbacks to using the Value startupScriptContent as the initialization script. For example, if the localstack helm chart is a subchart, rendering startupScriptContent doesn't have access to Values or Files from the parent chart. In this case, it's necessary to mount the initialization script manually, for example:

localstack:
  enableStartupScripts: false  # We mount the startup script manually below
  volumeMounts:
    - name: localstack-init-scripts-config
      mountPath: /docker-entrypoint-initaws.d
  volumes:
    - name: localstack-init-scripts-config
      configMap:
        name: my-custom-localstack-init-scripts

It would be handy if we could mount a custom ConfigMap into localstack's /etc/localstack/init/ready.d directory. For example:

  enableStartupScripts: true
  startupScriptConfigMap: my-custom-localstack-init-scripts

would mount the given ConfigMap to the init script entry point.

@alexrashed alexrashed added the type: enhancement Improvement to usability or performance label May 5, 2023
@luispabon
Copy link

Didn't the init scripts at /docker-entrypoint-initaws.d get deprecated and removed on localstack 2.0 btw?

@sobotklp
Copy link
Author

sobotklp commented May 5, 2023

I believe so, yes. For localstack 2.0, the mountPath would have to be /etc/localstack/init/ready.d. Hiding that implementation detail is another benefit of this feature.

@luispabon
Copy link

The problem is that the feature itself on the chart isn't enough to express the different classes of init scripts, nor does it actually place them into /etc/localstack/init/ready.d, legacy-like. Since the chart doesn't really know what version you're installing as the default tag is latest. I personally reckon the feature would need to be stripped out, and document the volume config you pasted above. It's a more flexible solution in any case.

@alexrashed
Copy link
Member

I do agree, that this would be quite complicated with the different stages. But on the other hand, most of the init scripts are for the ready stage, which is why I would like to keep the feature for these. I already added this to #81 with 35f0d37.
However, I don't think it's worth it to completely map the different stages to the helm chart, while having the volumeMounts feature where everything can be easily mapped.

Considering the specific feature itself - having the possibility to define a configmap instead of using the startupScriptContent - I'm not too convinced yet that the benefit outweighs the additional complexity.
An additional - backwards compatible - change would be to only create the configmap if startupScriptContent is not empty.
This would allow you to create a configmap with the name {{ template "localstack.fullname" . }}-init-scripts-config manually.
However, this would be a bit implicit (but this could be tackled with additional documentation).

I believe so, yes. For localstack 2.0, the mountPath would have to be /etc/localstack/init/ready.d. Hiding that implementation detail is another benefit of this feature.

This structure is considered an API (and not an implementation detail) for LocalStack, which means that this will not break between major releases.

@luispabon
Copy link

Being able to create the configmap ourselves would be ideal, as values files in helm can't accept templating and often the contents on that configmap are non-trivial. Being able to pass the chart the names of those configmaps for each kind of init script would be the cherry on the top, instead of following a naming convention. I'm a big believer on explicit, not implicit, configuration. But this is all mi opinion.

@luispabon
Copy link

Also I do not think it's unreasonable to make a BC break here on the chart, and bump major version. I don't think it's necessary to saddle ourselves with an ill-fitting api when semver is available.

@alexrashed
Copy link
Member

For this chart, there is no semantic versioning yet, because we are still in 0 (as per the spec of semantic versioning, but we might as well decide to stay in zero-ver for a while).
Currently we do not plan to create a major release of this chart, for now we want to keep the changes backwards compatible.
But backwards compatible or not, I think it might just be an overkill to add all stages here to the values, when we have all the flexibility in the world with the volumes config.

Do you see a nice way of getting the best of both worlds? How would the values.yaml look like that supports both, the nice and simple way of adding the script content in the values file with startupScriptContent as well as the possibility to define an external configmap?

...instead of following a naming convention

I really want to stress (as above), that this is not a naming convention but part of the user-facing configuration API of LocalStack. LocalStack - in contrast to this chart - is being versioned according to semantic versioning. These changes will not break in between major releases (while technically this chart would be allowed to contain breaking changes in any version, even though we don't do it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement to usability or performance
Projects
None yet
Development

No branches or pull requests

3 participants