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

feat(onboarding): Add onboarding values to the helm chart #510

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

tfadeyi
Copy link
Contributor

@tfadeyi tfadeyi commented Feb 5, 2024

Adds cluster onboarding values to the agent helm chart.

Change:

  • Add onboarding object to the values.yaml
  • Add onboarding.clusterName which updates the agent configMap template
  • Add onboarding.clusterDescription which updates the agent configMap template
  • Update Autogen docs

Note: Wait until v0.1.44 agent is released before merging

@hawksight
Copy link
Contributor

hawksight commented Feb 5, 2024

@tfadeyi I had a couple of q's for context:

  • What is the significance of "onboarding"?
  • Could these keys just be under config.clusterName & config.clusterDescription?
  • Are the two fields clusterName & clusterDescription optional?

@tfadeyi
Copy link
Contributor Author

tfadeyi commented Feb 5, 2024

@tfadeyi I had a couple of q's for context:

* What is the significance of "onboarding"?

We are adding these fields in order to allow users to configure the necessary fields in order to connect/onboard a cluster to VCP without going through UI or CLI. https://venafi.atlassian.net/browse/VC-22924

image

Adding those fields will make the backend create a new cluster connection element in VCP.

* Could these keys just be under `config.clusterName` & `config.clusterDescription`?

That's true the could be, just thought it would make it more clear that these fields would be used to onboard a cluster to VCP.

* Are the two fields `clusterName` & `clusterDescription` optional?

Yeah these fields are optional

Hopefully this https://venafi.atlassian.net/browse/VC-22924 will give more context on the problem

@achuchev
Copy link

achuchev commented Feb 5, 2024

@hawksight All good points. Let me know if you want to understand more about the proposed change.

@tfadeyi I agree with Peter that the new options have to be under config.

@tfadeyi tfadeyi force-pushed the add-cluster-description-helm branch 2 times, most recently from d665b31 to 7a3b6e1 Compare February 6, 2024 10:11
Adds cluster onboarding values to the agent helm chart.

Signed-off-by: Oluwole Fadeyi <[email protected]>
@tfadeyi tfadeyi force-pushed the add-cluster-description-helm branch from 7a3b6e1 to 615ae2b Compare February 6, 2024 10:24
@hawksight
Copy link
Contributor

@tfadeyi thanks for the explanation. What would happen if somewhere were to change the clusterName and clusterDescription fields? They would just change on the VCP?

I was thinking that if they are a "set once and never change" type field, then it makes sense to consolidate them under their own heading onboarding. But if they are just optional and can be changed on an ongoing basis, then I think it makes more sense to have them sit alongside the other configurable variables:

helm show values oci://$VENAFI_REGISTRY_PRIVATE_US/charts/venafi-kubernetes-agent | yq .
config:
  server: "https://api.venafi.cloud/"
  clientId: "XXXXXXXXXXXXXXX"
  clusterName: "black-pearl"
  clusterDescription: "The fastest cluster on the seven seas"

It also feels a little more natural and consistent with the jetstack-agent config.

Also thinking about the user install experience, it just makes the command a tiny bit shorter:

# This
helm diff upgrade --install venafi-kubernetes-agent oci://$VENAFI_REGISTRY_PRIVATE_US/charts/venafi-kubernetes-agent --namespace $VENAFI_NAMESPACE --create-namespace --set config.clientId="$VENAFI_CLIENT_ID" --set config.clusterName="abcdefg" --set config.clusterDescription="Alphabet" --version "0.1.43" --registry-config "${VENAFI_CREDS_LOCAL_PATH}/${VENAFI_CRED_NAME}/config.json"
# As opposed to this:
helm diff upgrade --install venafi-kubernetes-agent oci://$VENAFI_REGISTRY_PRIVATE_US/charts/venafi-kubernetes-agent --namespace $VENAFI_NAMESPACE --create-namespace --set config.clientId="$VENAFI_CLIENT_ID" --set config.onboarding.clusterName="abcdefg" --set config.onboarding.clusterDescription="Alphabet" --version "0.1.43" --registry-config "${VENAFI_CREDS_LOCAL_PATH}/${VENAFI_CRED_NAME}/config.json"

Other note

Perhaps we should remove the chart bump from here and here and do that explicitly as it's own commit before creating a tag / release on this repo?

@achuchev
Copy link

achuchev commented Feb 6, 2024

@hawksight: The cluster name and description will be preserved in VCP even if the agent reports new values. This ensures consistency and avoids unintended updates.

@tfadeyi
Copy link
Contributor Author

tfadeyi commented Feb 6, 2024

Perhaps we should remove the chart bump from #502 and here and do that explicitly as it's own commit before creating a tag / release on this repo?

I've already made a new release, didn't see the message in time. I'll make a new release

@tfadeyi tfadeyi merged commit 15c631a into master Feb 6, 2024
8 checks passed
@inteon inteon deleted the add-cluster-description-helm branch September 16, 2024 14:14
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.

4 participants