Skip to content

chore: add new workflow to notify if nrdot artifacts match AC expectations#297

Closed
alvarocabanas wants to merge 1 commit intonewrelic:mainfrom
alvarocabanas:main
Closed

chore: add new workflow to notify if nrdot artifacts match AC expectations#297
alvarocabanas wants to merge 1 commit intonewrelic:mainfrom
alvarocabanas:main

Conversation

@alvarocabanas
Copy link
Contributor

Workflow triggered on commit or PR to main that checks the integrity of the artifacts used by Agent Control when embedding the NRDOT on host binary and config.

Checks:

  • Ensure config.yaml file exists in current path
  • Check for config.yaml is not modified in last commit.
  • Check binary name in goreleaser file matches expected.
  • Check package name in goreleaser file matches expected.

If any of this checks doesn't pass a message is sent to AC and nrdot slack channels to be reviewed. But the workflow doesn't fail.

@alvarocabanas alvarocabanas requested a review from a team as a code owner April 10, 2025 08:41
fi
echo "failure_message=$failure_message" >> $GITHUB_OUTPUT

notify-conflicts:
Copy link
Contributor

Choose a reason for hiding this comment

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

why not making the workflow fail instead of notifying ?
Is it desirable that we brake this contract?

Copy link
Contributor Author

@alvarocabanas alvarocabanas Apr 10, 2025

Choose a reason for hiding this comment

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

@kb-newrelic is that something you could contemplate?
Of course then we shouldn't fail on config.yaml modification and only notify of a change so we can check it, but for the other cases, could we fail the pipeline?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree failing a PR for violating the basic requirements (path, binary/package name) is reasonable. As we already have a sync/check for the k8s distro, I created a PR to follow the same pattern but it was super helpful to have yours as a template, so I appreciate the work you put in!

As you have already pointed out, we can't have the same strictness for the config as it would create a pretty limiting dependency.

I was hoping we could codify the dependency on the config a bit more clearly to avoid having to start the analysis from scratch anytime this config changes. Like if we were to change the memorylimiterprocessor defaults, I assume it would be unnecessary to alert either one of our teams but I need to understand the exact dependency better to codify it properly.
Things that come to mind would be:

  • any change in env vars names (removal, newly added, renames)
  • host entity synthesis (out of scope for this - it's in our backlog to validate that in our E2E tests though)
  • ...?

If your team wants to be notified of any change, we can for sure do that but for our team it would be a non-actionable alert, so I would like to avoid that.

Copy link
Contributor

@kb-newrelic kb-newrelic Apr 14, 2025

Choose a reason for hiding this comment

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

We can decide in this thread what to do and I will replace this TODO with whatever we end up with.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added env var assertions: 4e94d7f

@kb-newrelic
Copy link
Contributor

kb-newrelic commented Apr 14, 2025

Closing this in favor of #298, see also this comment

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

Comments