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(alertsNotificationChannel): generate alerts API #811

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

smcavallo
Copy link

@smcavallo smcavallo commented Sep 17, 2021

Can someone please advise here? Is this PR on the right track?
Would like to add support for managing notification channels using the nerdgraph API -> https://docs.newrelic.com/docs/alerts-applied-intelligence/new-relic-alerts/advanced-alerts/alerts-nerdgraph/nerdgraph-api-notification-channels/
Whenever I build locally the generator is unable to generate any of the nerdgraph/*_api.go files (they are all empty) - with the exception thrown below.
The types are generated but not the plumbing for the api calls.

ERROR  [2021-09-16T23:07:32Z] failed to call Execute() for provider string: template: client.go.tmpl:52: function "hasField" not defined
pkg/alerts/alerts_api.go:1:1: expected 'package', found 'EOF'
pkg/cloud/cloud_api.go:1:1: expected 'package', found 'EOF'
=== app === [ generate         ]: Running generate...
=== app === [ deps             ]: Installing package dependencies required by the project...
=== app === [ spell-check-fix  ]: Fixing spelling mistakes with misspell...
=== app === [ gofmt-fix        ]: Fixing file format with gofmt...
pkg/alerts/alerts_api.go:1:1: expected ';', found 'EOF'
pkg/alerts/alerts_api.go:1:1: expected 'IDENT', found 'EOF'
pkg/alerts/alerts_api.go:1:1: expected 'package', found 'EOF'
pkg/cloud/cloud_api.go:1:1: expected ';', found 'EOF'
pkg/cloud/cloud_api.go:1:1: expected 'IDENT', found 'EOF'
pkg/cloud/cloud_api.go:1:1: expected 'package', found 'EOF'
make: *** [build/lint.mk:45: gofmt-fix] Error 123

Signed-off-by: smcavallo [email protected]

@CLAassistant
Copy link

CLAassistant commented Sep 17, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ smcavallo
❌ sanderblue
You have signed the CLA already but the status is still pending? Let us recheck it.

@sanderblue
Copy link
Contributor

Hi @smcavallo, yes this looks like you're on the right track, however, the client already has some notification channel features that were added prior to us building and using tutone for code generation. So there might be some issues with manually written code vs generated code. That being said, I don't see an alerts_api.go file yet, so that might not be an issue.

You might try a few things just to make sure your setup is good to go though. You can also try starting with a small scope of tutone work by only adding 1 mutation to start and work your way through the rest.

  1. Ensure you're on the latest version of tutone. Unfortunately at the moment, this might involve cloning the repo and running make compile-only and then using the locally compiled binary (e.g. /path/to/repo/bin/darwin/tutone). However, if tutone has been working in general for you, then you may not need to do this.

  2. Only add one mutation to start. I suggest starting with the simpler one - maybe alertsNotificationChannelDelete or alertsNotificationChannelCreate just to get things started.

  3. Run tutone generate --refetch --package alerts
    This will force a refetch of the GraphQL schema and then will only target the alerts package (i.e. ./pkg/alerts) as so not interfere with other packages, which can cause some unnecessary noise.

If that doesn't work, then there's a chance there's something wonky with the schema or there's a bug in tutone for a unique situation. At a minimum, you could add the types via tutone and write the implementation following previous patterns in other generated files. Hope this helps 🙂

@smcavallo smcavallo marked this pull request as ready for review September 30, 2021 18:19
@smcavallo
Copy link
Author

@sanderblue - thanks, that worked. had to rebuild tutone from latest, and ran the following command
/tutone/bin/linux/tutone -c .tutone.yml generate --refetch --package alerts
otherwise it kept using tutone version dev and not working properly

@sanderblue
Copy link
Contributor

Hi @smcavallo, glad that worked. It looks like the lint stage of CI is failing. You will need to reformat your commit message per the format described in contribution guide 🙂

@smcavallo smcavallo changed the title nerdgraph alertsNotificationChannel support feat(alertsNotificationChannel): generate alerts API Oct 14, 2021
@smcavallo
Copy link
Author

@sanderblue - updated the commit message, thanks again!

@sanderblue
Copy link
Contributor

@smcavallo Not sure what's up here, but it looks like there's a reference to an undeclared type named AlertsNotificationChannelMutation. I noticed it in the Tutone config, but I'm not quite sure why this isn't defined somewhere. You might be able to remove that line from the config because Tutune should be able to recognize it as part of the response object. Then rerun the code generation to see if that worked. After regenerating the code, run make lint locally on your machine to see if that passes. If it does, then I would try running make test-unit before committing just to make sure unit tests pass locally. Sorry this has taken a bit to get working. Hopefully we can figure it out with this last round.

@smcavallo
Copy link
Author

@sanderblue - thanks for the feedback, it was super helpful. AlertsNotificationChannelMutation was a UNION so tutone was not able to map it to a type which resulted in the following warning when generating:

WARNING[2021-11-07T16:52:34Z] kind not implemented                          kind=UNION name=AlertsNotificationChannelMutation

So instead I used field_type_override: AlertsNotificationChannel which looks like the right type which should be used and it looks much better now - the types/linter/tests look good. Ready for the workflow to be re-run if you wouldn't mind kicking it off.

@sanderblue
Copy link
Contributor

Thanks for the heads up regarding the UNION type, and good call with the field_type_override usage there. Have you been able to confirm this works as you expect?

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