-
Notifications
You must be signed in to change notification settings - Fork 301
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
Artifact creation and validation #2540
Artifact creation and validation #2540
Conversation
Correct type definition pathing and datasourceIds type Co-authored-by: Zack Stickles <[email protected]>
StatusI have the validations running properly and outputting the errors to the console. Next steps would be:
|
Co-authored-by: Mickey Ryan <[email protected]> Co-authored-by: Sarah Kitten <[email protected]>
Co-authored-by: Zack Stickles <[email protected]> Co-authored-by: Sarah Kitten <[email protected]> Co-authored-by: Andrew Anguiano <[email protected]>
0294b60
to
5338b6b
Compare
Co-authored-by: Zack Stickles <[email protected]> Co-authored-by: Sarah Kitten <[email protected]> Co-authored-by: Andrew Anguiano <[email protected]>
f2782f4
to
a54677a
Compare
…newrelic/newrelic-quickstarts into campfire/NR-268461-build-validate-artifact
return alertPaths.map(alertPath => { | ||
// The identifier for alerts is the folder and the file name | ||
// e.g. `node-js/HighCpuUtilization.yml` | ||
const identifier = path.join(...alertPath.split('/').slice(-2, -1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not likely, but I always get a little anxious seeing slice
(or pop
) on arrays since those can technically return []
(and undefined
for pop
).
Typescript doesn't seem to be angry at this here, but I know that we're using a !
in the Quickstart file to get around this. We might want to consider some additional error checking (or tests) to ensure we for sure have an identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I wouldn't block the PR on this!
Co-authored-by: Joe Gregory <[email protected] Co-authored-by: Sarah Kitten <[email protected]>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found at least one (potential) bug and a few places where we might want to refactor. Additionally, we don't have any tests for this work (something called out in the acceptance criteria).
I'll probably jump in to make these adjustments since this is blocking other work.
I have addressed my own feedback. This is going to need someone else to review now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is easy to read and passes when run with the current config changes PR. Small suggestion for the tests but not blocking.
.fn() | ||
.mockReturnValueOnce([{ config: 'test-dataSource-1' }]); | ||
|
||
Alert.getAll = jest.fn().mockReturnValueOnce([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to test each of these with non-empty values here since we had problems with the getAll returning nothing before!
|
||
const value = get(artifact, invalidValuePath); | ||
|
||
// Get the specific "component" (e.g. the alert or dashboard) that contains |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate this refactoring and comments!
…github.com/newrelic/newrelic-quickstarts into campfire/NR-268461-build-validate-artifact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the author, I can't approve, but this looks good to me and functions well. I was originally hesitant to bring in lodash for a single use-case, but it does clean it up a bit and using the modularized version is nice 👍 ✅
This is based on #2538 and adds a script to create the release artifact and validate it.
Todo
Save artifact to filesystem (or in a way that can be published easily by the next step in the process)Set up GH action to run script