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

[super-agent] Improve helm chart UX (NR-281093) #1466

Merged
merged 16 commits into from
Sep 23, 2024

Conversation

kang-makes
Copy link
Contributor

@kang-makes kang-makes commented Aug 20, 2024

Is this a new chart

No

What this PR does / why we need it:

super-agent chart got the config from a field called content that added any arbitrary data to a config map. This is supposed to control this configMap and render it using values from Helm's values.yaml.

It also adds some protections so a customer is not able to use it on unsupported environments.

Special notes for your reviewer:

This PR is blocked until #1463 is merged.

Checklist

  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [mychartname])

@kang-makes kang-makes force-pushed the kang-makes/improve-helm-chart-ux/NR-281093 branch 2 times, most recently from 913128e to dde4cec Compare August 20, 2024 11:33
@kang-makes kang-makes force-pushed the kang-makes/improve-helm-chart-ux/NR-281093 branch 5 times, most recently from d286ec0 to 8d5ab66 Compare September 3, 2024 08:59
@kang-makes kang-makes marked this pull request as ready for review September 3, 2024 09:00
@kang-makes kang-makes requested review from a team as code owners September 3, 2024 09:00
@kang-makes kang-makes marked this pull request as draft September 3, 2024 09:04
@kang-makes kang-makes force-pushed the kang-makes/improve-helm-chart-ux/NR-281093 branch from 8d5ab66 to 04fe88d Compare September 3, 2024 12:20
@kang-makes
Copy link
Contributor Author

I had to remove from the chart the test for cluster's Kubernetes version as it was colliding with the CI so I moved it to the PR #1471

@kang-makes kang-makes marked this pull request as ready for review September 3, 2024 12:24
@kang-makes kang-makes force-pushed the kang-makes/improve-helm-chart-ux/NR-281093 branch 5 times, most recently from 0187446 to 2b9279a Compare September 4, 2024 07:25
@kang-makes
Copy link
Contributor Author

The upgrade path is failing with this error:

Error: UPGRADE FAILED: template: super-agent/charts/super-agent-deployment/templates/deployment-superagent.yaml:24:34: executing "super-agent/charts/super-agent-deployment/templates/deployment-superagent.yaml" at <include (print $.Template.BasePath "/configmap-superagent-config.yaml") .>: error calling include: template: super-agent/charts/super-agent-deployment/templates/configmap-superagent-config.yaml:12:8: executing "super-agent/charts/super-agent-deployment/templates/configmap-superagent-config.yaml" at <include "newrelic-super-agent.config.content" .>: error calling include: template: super-agent/charts/super-agent-deployment/templates/_helpers.tpl:137:33: executing "newrelic-super-agent.config.content" at <include "newrelic-super-agent.config.agents.yaml" .>: error calling include: template: super-agent/charts/super-agent-deployment/templates/_helpers.tpl:18:8: executing "newrelic-super-agent.config.agents.yaml" at <fail (printf "Agent %s does not have agent type" $subAgentName)>: error calling fail: Agent open-telemetry does not have agent type

The previous values.yaml had this hardcoded: https://github.com/newrelic/helm-charts/pull/1466/files#diff-3bd49393f47bdfcb35b90c33237f384109da730bc892fa317ba8db58d29d7975L149-L158

Now, instead of having it in the values we have it as a helper: https://github.com/newrelic/helm-charts/pull/1466/files#diff-f752c985c9b04f49290d4a210d58d0981e8197c5825b7747defdc701fa4e1652R25-R34

The schema changed from this:

    superAgent:
      content:
        agents:
          open-telemetry:
            agent_type: newrelic/io.opentelemetry.collector:0.2.0
    subAgents:
      open-telemetry:
        content:
          chart_values:
            licenseKey: ${nr-env:NR_LICENSE_KEY}
            cluster: ${nr-env:NR_CLUSTER_NAME}

To this:

    subAgents:
      open-telemetry:
        type: newrelic/io.opentelemetry.collector:0.2.0
        content:
          chart_values:
            licenseKey: ${nr-env:NR_LICENSE_KEY}
            cluster: ${nr-env:NR_CLUSTER_NAME}

So the new chart is complaining that the old chart does not have type on the subagent.

The test is ok to fail as this is a breaking change. It should not have any repercussion to any customer as we do not support upgrades with --reuse-values.

@kang-makes kang-makes force-pushed the kang-makes/improve-helm-chart-ux/NR-281093 branch from 2b9279a to e9a9af8 Compare September 4, 2024 14:49

# -- Values that the fleet is going to have in the deployment.
# @default -- See `values.yaml` for examples
# @default -- {} (Empty. That defaults to configure the `newrelic/io.opentelemetry.collector` subagent)
Copy link
Member

Choose a reason for hiding this comment

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

I think that this is not clear if you do not know what it is going on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the clearest way I found to explain it. I am open to any suggestion because it is a weird default.

Copy link
Member

Choose a reason for hiding this comment

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

I think that if we say something: "If empty the chart add automatically the newrelic/io.opentelemetry.collector subagent. On the other hand, if populated the list of agent created is the one specified overwriting the default"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #f11ce5a. Let me know what you think :)

charts/super-agent/values.yaml Outdated Show resolved Hide resolved
charts/super-agent/values.yaml Outdated Show resolved Hide resolved
charts/super-agent/Chart.yaml Show resolved Hide resolved
@kang-makes kang-makes requested review from gsanchezgavier and a team September 19, 2024 09:10
gsanchezgavier
gsanchezgavier previously approved these changes Sep 19, 2024
Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

I just left nits, feel free to discard them, LGTM thanks!

@kang-makes kang-makes force-pushed the kang-makes/improve-helm-chart-ux/NR-281093 branch from 833325e to 34b7dc2 Compare September 23, 2024 09:22
@@ -62,7 +101,7 @@ spec:
ERROR_MESSAGE=$(jq -r '.errors[0].message // "NOERROR"' "$TEMPORAL_FOLDER/response.json")
if [ "$ERROR_MESSAGE" != "NOERROR" ]; then
echo "Error creating an identity: $ERROR_MESSAGE"
echo "Failed to create a New Relic System Identity for OpAMP communication authentication. Please verify that your User Key is valid and that your Account Organization has the necessary permissions to create a System Identity: $ERROR_MESSAGE"
Copy link
Contributor

Choose a reason for hiding this comment

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

cool thanks!

@kang-makes kang-makes merged commit aa58971 into master Sep 23, 2024
10 of 11 checks passed
@kang-makes kang-makes deleted the kang-makes/improve-helm-chart-ux/NR-281093 branch September 23, 2024 10:17
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