Fix connect pod service account values when launcher is enabled

[dbkegley]

Fixes #584

The connect pod service account values were being incorrectly applied when the job launcher is enabled. This PR fixes the Connect pod's serviceAccount value and removes some dead code.

Before:

# w/o launcher
» helm template ./ --set rbac.serviceAccount.name=asdf | grep serviceAccount 
      serviceAccountName: "asdf"

# w/ launcher
» helm template ./ --set launcher.enabled=true --set sharedStorage.create=true --set rbac.serviceAccount.name=asdf | grep serviceAccount
   ...
      serviceAccountName: release-name-rstudio-connect

After:

# w/o launcher
» helm template ./ --set rbac.serviceAccount.name=asdf | grep serviceAccount 
      serviceAccountName: "asdf"

# w/ launcher
» helm template ./ --set launcher.enabled=true --set sharedStorage.create=true --set rbac.serviceAccount.name=asdf | grep serviceAccount
...
      serviceAccountName: asdf

[dbkegley]

pod.serviceAccountName is already fully deprecated

» helm template ./ --set pod.serviceAccountName=asdf                  
Error: execution error at (rstudio-connect/templates/NOTES.txt:55:4): 

`pod.serviceAccountName` is no longer used. Use `rbac.serviceAccount.name` instead.

Use --debug flag to render out invalid YAML

[zackverham]

Not immediately related to your PR itself - but it does feel like something in CI should have caught this, yeah?

[jforest]

Not immediately related to your PR itself - but it does feel like something in CI should have caught this, yeah?

There are not many ct install tests yet for each product, so we would not have had any way to catch this yet. Hopefully @dbkegley and others can help with creating the values files needed in ci/<product_name>/install/<testname>-values.yaml as we continue. We are working on it though!

[dbkegley]

@jforest I'm not very familiar with chart-testing but do you know if it would allow us to provide assertions for expected output? This bug for example would have produced a "valid" installation but would have used the wrong service account for the Connect pod. I'm happy to help look into this more myself as well, just wondering if you know off hand

[jforest]

@jforest I'm not very familiar with chart-testing but do you know if it would allow us to provide assertions for expected output? This bug for example would have produced a "valid" installation but would have used the wrong service account for the Connect pod. I'm happy to help look into this more myself as well, just wondering if you know off hand

I don't think so, I believe you can create a values file (ending in -values.yaml) and run an install with those values files.

Could you merge main into this branch please? It should fix the linting error

[dbkegley]

I don't think so, I believe you can create a values file (ending in -values.yaml) and run an install with those values files.

That makes sense, seems like we have the install half but are missing the assertions. I believe we just need to write some tests that live in the charts/rstudio-connect/templates/test directory and it sounds like they will get evaluated automatically by each ct install

https://helm.sh/docs/topics/chart_tests/
https://github.com/helm/chart-testing/blob/main/doc/ct_install.md#synopsis

cc @plascaray

Could you merge main into this branch please? It should fix the linting error

Done!

[dbkegley]

@jforest would you mind taking another look at this CI failure? I pulled from main and the branch seems to have passed all the linting but it's failing to add the linting results to the PR with the following error:

SyntaxError: Unexpected identifier 'pod'

[jforest]

@jforest would you mind taking another look at this CI failure? I pulled from main and the branch seems to have passed all the linting but it's failing to add the linting results to the PR with the following error:

SyntaxError: Unexpected identifier 'pod'

Yah, I've got a test PR trying to fix this, I think I'm just going to pull out trying to post the output to the PR, it's causing more trouble than it's worth. People can just check the GHA output directly

I'll let you know when I've gotten that merged to main so you can merge main again

[jforest]

@dbkegley I have merged #597, you should merge main into your branch!

[dbkegley]

Thanks @jforest ! It's working now, would you mind reviewing/approving for infraops so we can get this fix released?

[jforest]

The changes of order here are just to ensure that if we have .Values.rbac.serviceAccount.name set, it will override the default of rstudio-connect.fullname?

[dbkegley]

Yep, that's exactly right