Skip to content

Add install configmap override flag#47

Merged
merenbach merged 38 commits intoargoproj:masterfrom
merenbach:add-tls-and-auth-support
Mar 22, 2018
Merged

Add install configmap override flag#47
merenbach merged 38 commits intoargoproj:masterfrom
merenbach:add-tls-and-auth-support

Conversation

@merenbach
Copy link
Copy Markdown
Contributor

@merenbach merenbach commented Mar 20, 2018

Background

To get us moving on issue #29, we need to be able to configure the installed server with the name of a config map for settings overrides, including the name of a secret to evaluate against the admin password on attempted login.

Problem

No flags currently exist to provide a configmap override on install, and no generic retrieval for secrets or config maps is currently implemented.

Solution

Add a configmap override for install and argocd-server invocation, along with util methods (and matching tests!) to administer config maps and secrets.

@merenbach merenbach requested a review from alexmt March 20, 2018 21:30
@merenbach merenbach changed the title Add install configmap override and utils for secrets/configmaps Add install configmap override Mar 21, 2018
@merenbach merenbach added the help-wanted Extra attention is needed label Mar 21, 2018
@merenbach merenbach closed this Mar 21, 2018
@merenbach merenbach reopened this Mar 21, 2018
@merenbach merenbach removed the help-wanted Extra attention is needed label Mar 21, 2018
@merenbach merenbach added the help-wanted Extra attention is needed label Mar 21, 2018
@merenbach merenbach changed the title Add install configmap override Add install configmap override flag Mar 21, 2018
@merenbach merenbach removed the help-wanted Extra attention is needed label Mar 21, 2018
// Use a Kubernetes ConfigMap, if provided.
if i.InstallOptions.ConfigMap != "" {
configMap := strconv.Quote(i.InstallOptions.ConfigMap)
container := &argoCDServerControllerDeployment.Spec.Template.Spec.InitContainers[0]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use Containers instead of InitContainers.

)

// ConfigManager holds config info for a new manager with which to access Kubernetes ConfigMaps.
type ConfigManager struct {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Right now argocd only need to read settings from config map and referenced secrets. We don't have use for create/update methods. I think ConfigManager should be responsible to deserialize Argo CD settings structure from config/secrets. E.g. now it needs only one public method GetSettings which encapsulate settings deserialization:

type ConfigManager struct {
   clientset kubernetes.Interface
   configMapName string
}

func (mgr *ConfigManager) GetSettings() ArgoCDSettigns {
   ....
}

server/server.go Outdated
endpoint = fmt.Sprintf("localhost:%d", port)
)

// ArgoCDServerSettings holds in-memory runtime configuration options.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As we discussed we will need to add more hardcoded parameters into settings structure (e.g. app resync timeout). These parameters are not specific to argocd server, so I would suggest to move settings struct definition into util package next to config manager

Copy link
Copy Markdown
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

@merenbach merenbach merged commit b1682cc into argoproj:master Mar 22, 2018
alexec pushed a commit that referenced this pull request Apr 24, 2019
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Sep 29, 2023
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