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

[#135] Allow specifying the mount path for ConfigMaps #136

Closed
wants to merge 2 commits into from

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Feb 24, 2020

This PR will allow the configuration of ConfigMpas by defining a MountPath in addition to the Name

The MountPath could be a relative or an absolute path. If it is a relative path, then it is treated as relative to JBOSS_HOME. The MountPath is optional, and if it is not specified then it defaults to /etc/configmaps/

This fixes #135

Copy link
Member

@jmesnil jmesnil left a comment

Choose a reason for hiding this comment

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

The new feature itself looks good, thanks!

I've requested change to keep the code consistent but I'm fine with the new code.

deploy/crds/wildfly_v1alpha1_wildflyserver_crd.yaml Outdated Show resolved Hide resolved
doc/apis.adoc Show resolved Hide resolved
doc/user-guide.adoc Outdated Show resolved Hide resolved
pkg/apis/wildfly/v1alpha1/wildflyserver_types.go Outdated Show resolved Hide resolved
// +k8s:openapi-gen=true
type ConfigMapSpec struct {
Name string `json:"name"`
MountPath string `json:"mountPath,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

could you please move the comment about the mount path from ConfigMapSpec to here so that is is correctly picked when the openapi and k8s resources are generated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pkg/apis/wildfly/v1alpha1/wildflyserver_types.go Outdated Show resolved Hide resolved
pkg/resources/statefulsets/statefulset.go Outdated Show resolved Hide resolved
pkg/apis/wildfly/v1alpha1/wildflyserver_types.go Outdated Show resolved Hide resolved
@yersan yersan requested a review from jmesnil March 30, 2020 15:03
@pkremens
Copy link

@yersan @jmesnil @mchoma @jbliznak I don't think we can do it that way. This is a way we use to setup the configMaps field in EAP Operator (registry-proxy.engineering.redhat.com/rh-osbs/jboss-eap-7-eap73-rhel8-operator:1.0-8)

  configMaps:
  - config-map-name

After the change, the same setup would be

    configMaps:
    - name: config-map-name

Trying to use the old style config with this version of operator gives me

# * spec.configMaps: Invalid value: "string": spec.configMaps in body must be of type object: "string"

thus we're breaking the backward compatibility with this one.

@jmesnil
Copy link
Member

jmesnil commented Apr 28, 2020

That's correct, this feature would break backwards compatibility.
Note that the CRD apiVersion is wildfly.org/v1alpha1. We have clearly identified that our API is in an alpha state and might be changed without backwards compatibility.

We should strive to preserve backwards compatibility when introducing a new feature but in that specific case, I prefer we provide a clean representation of a config map (with a name and an optional location) rather than preserve the backwards compatibility with a clunky api.
When we discussed this feature with @yersan we could not find a correct representation for that feature that would preserve backwards compatibility so I ok-ed to break it.

@jmesnil
Copy link
Member

jmesnil commented Apr 28, 2021

let's close this PR as we will need first to have a conversion web hook to introduce this change without breaking compatibility

@jmesnil jmesnil closed this Apr 28, 2021
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.

With operator it is not possible to mount configMap to $JBOSS_HOME/extensions
3 participants