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

Removes unused values from helm chart #29

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ashtongraves
Copy link
Contributor

No description provided.

@ashtongraves
Copy link
Contributor Author

@brianhlin @matyasselmeci can one of you give this a look over? Particularly look at the slate section, I wasn't confident if there was still bits being used, I just kind of ripped out everything that referenced slate. Please give it a look over and let me know if there's things we need to keep but rename instead.

@ashtongraves
Copy link
Contributor Author

Also is that _helpers.tpl file still used?

Copy link
Contributor

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

One big issue and a few minor nitpicks. I believe we still use _helpers.tpl

supported/osg-htc/osg-hosted-ce/Chart.yaml Outdated Show resolved Hide resolved
supported/osg-htc/osg-hosted-ce/templates/configmap.yaml Outdated Show resolved Hide resolved
supported/osg-htc/osg-hosted-ce/values.yaml Outdated Show resolved Hide resolved
@ashtongraves
Copy link
Contributor Author

@brianhlin can you give this another look over? Also don't merge yet, Jeff D had some concerns with the dashboard piece. Want to make sure he's okay with changes before merge

@brianhlin
Copy link
Contributor

FWIW, I added the dashboard piece based on what I thought what Miron wanted but he's decided to go in a separate direction wrt the dashboard.

Copy link
Contributor

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

There's one thing that you should cross-check on the container side but otherwise LGTM

labels:
app: osg-hosted-ce
instance: {{ .Values.Instance }}
release: {{ .Release.Name }}
app.kubernetes.io/part-of: {{ .Chart.Name }}
app.kubernetes.io/instance: {{ .Release.Name }}
data:
50-slate-scitokens.conf: |+
50-scitokens.conf: |+
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth looking in the container to make sure there isn't anything hardcoded referencing this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we're good as the CE container images look at the entire mapfiles.d dir for mappings to determine which users need to be provisioned: https://github.com/opensciencegrid/docker-compute-entrypoint/blob/master/base/etc/osg/image-config.d/ce-common-startup

The default conf that you link is mostly intended for admins setting up their own local CE and notice that there aren't any uncommented lines. The entire mapfiles.d dir is parsed in alphanumeric order and concatenated together to get credential -> user mappings (the file that determines what's included is /etc/condor-ce/mapfiles.d).

You'll see this pattern used a lot for configuration, e.g. conf.d dirs, across different Linux applications.

@ashtongraves
Copy link
Contributor Author

One big issue and a few minor nitpicks. I believe we still use _helpers.tpl

What exactly does that helpers.tpl do? Like it seemed like that line I removed wasn't used right?

@brianhlin
Copy link
Contributor

_helpers.tpl contains named templates: https://helm.sh/docs/chart_template_guide/named_templates/. If you're sure they're not used, you could remove them but it certainly doesn't hurt much if they stick around

@ashtongraves
Copy link
Contributor Author

_helpers.tpl contains named templates: https://helm.sh/docs/chart_template_guide/named_templates/. If you're sure they're not used, you could remove them but it certainly doesn't hurt much if they stick around

I'm not sure, I just know for the part I removed, I haven't seen anything namespace related defined with slate-vo- prefix. In fact Looking through the helm chart and our helm release, I don't see anything set for a namespace so not sure where .Release.namespace is coming from, but I'm assuming it was removed at some point and this piece wasn't cleaned up?

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