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

Fix mounting for inputs container #642

Merged
merged 6 commits into from
Apr 27, 2023
Merged

Fix mounting for inputs container #642

merged 6 commits into from
Apr 27, 2023

Conversation

jsaun
Copy link
Contributor

@jsaun jsaun commented Apr 17, 2023

No description provided.

@jsaun jsaun requested a review from MattMcL4475 April 17, 2023 19:29
var valuesString = KubernetesYaml.Serialize(values);
await File.WriteAllTextAsync(TempHelmValuesYamlPath, valuesString);
await Deployer.UploadTextToStorageAccountAsync(storageAccount, Deployer.ConfigurationContainerName, "aksValues.yaml", valuesString, cts.Token);
}

private void UpdateValues(HelmValues values, Version previousVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this method should be named something like "UpdateContainerValues", and then in the Deployer.cs, there is a version check in the Update area, which would result in calling this. That way it's very clear what happens during an update, not buried in KubernetesManager.cs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was that all version related updates to the helm values would be added to this method not just this example for containers. I renamed the method to ProcessHelmValuesUpdates though, and passed the installedVersion from the deployer so hopefully its a little clearer.

{{- range .Values.internalContainersMIAuth }}
{{- $path := printf "/%s/%s" .accountName .containerName }}
{{- $claimName := printf "%s-%s-claim1" .accountName .containerName }}
{{- if eq $storageAccount .accountName }}
{{- if and (eq $storageAccount .accountName) (has .containerName $cromwellContainers)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this case insensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not, there is no case insensitive comparison in the helm template language from what I can tell. I converted the strings to lowercase for comparison now though.

@jsaun jsaun force-pushed the jsaun/fix-inputs branch from 29a6199 to 8f89ad2 Compare April 27, 2023 01:07
@jsaun jsaun requested a review from MattMcL4475 April 27, 2023 02:38
@jsaun jsaun merged commit 640b971 into main Apr 27, 2023
@jsaun jsaun deleted the jsaun/fix-inputs branch April 27, 2023 18:49
@ngambani ngambani added this to the 4.3.0 milestone Apr 28, 2023
@MattMcL4475
Copy link
Contributor

@jsaun can you please link the original issue and/or update the title and description of this PR?

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.

3 participants